-
Notifications
You must be signed in to change notification settings - Fork 7
feat: addition of the APIRequest example in Elixir folder #21
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I found a few things here that should be addressed. I think as it stands now this falls into the category of a "demo" rather than "example". I left a more detailed explanation about that in the review - the choice is yours.
I have a PR waiting for a review that will add "reuse" checks (license and copyright compliance), you are missing this in a few of the files (don't worry about .gitignore
it is fixed in the open PR).
elixir/APIRequest/lib/APIRequest.ex
Outdated
got_ip: fn info -> :io.format(~c"Got IP: ~p~n", [info]) end, | ||
disconnected: fn -> :io.format(~c"WiFi disconnected~n") end, | ||
# Edit these values for your network: | ||
ssid: ~c"SSID_NAME", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We don't want hard coded credentials in the app. If someone makes a PR to update this PR in the future (maybe to accommodate AtomVM API updates) it would be easy for someone to accidentally push their credentials to GitHub. These values should be read from a config file (which a template should be provided for... I.E. config.exs-template) that should be edited by end users a renamed to the actual config file name (config.exs). The real configuration file should be added to .gitignore
so credentials can never be accidentally pushed to GitHub.
defp start_network() do | ||
:io.format(~c"Starting network...~n") | ||
|
||
config = [ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You could choose to make this a demo, but if you would like to keep it as a simple example instead, I recommend using : wait_for_sta/1
(or /2) and not use the callbacks. The goal for the examples is to keep them to a very limited scope. If you would like to submit a separate PR with an Elixir example for connecting to WiFi and using the callbacks (like the Erlang wifi example) that would be a very welcome addition too!
elixir/Blinky/mix.exs
Outdated
@@ -10,7 +10,7 @@ defmodule Blinky.MixProject do | |||
deps: deps(), | |||
atomvm: [ | |||
start: Blinky, | |||
flash_offset: 0x210000 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should make this correction in a separate PR.
elixir/HelloWorld/mix.exs
Outdated
@@ -10,7 +10,7 @@ defmodule HelloWorld.MixProject do | |||
deps: deps(), | |||
atomvm: [ | |||
start: HelloWorld, | |||
flash_offset: 0x210000 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be in a separate PR (it can be together with the above correction) we just want to keep the new additions in a different commit from the corrections to other examples. It makes the commit log cleaner and more useful.
elixir/APIRequest/lib/APIRequest.ex
Outdated
# | ||
# This file is part of AtomVM. | ||
# | ||
# Copyright 2018 Davide Bettio <davide@uninstall.it> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You wrote this, you should give yourself credit here ;-)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe we can make the README just a little more clear.
|
||
Welcome to the `APIRequest` AtomVM application. | ||
|
||
The `APIRequest` AtomVM application connects to a wifi, and when the connection is established retrieves and prints on console a GET request over https. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe we could change this to "...is established makes a GET request over https and prints the result to the console." Reading this I half expected the ESP32 to be the server getting the API request from a client, and printing the actual request on a the console.
That would be a strange example, but there were enough details left out to make it hard to be certain which details were missing, especially for non-native English speakers. ;-)
We merged the pending "reuse" workflow, so if you could rebase your changes we can run that check to make sure that others can freely use this example. |
In order to use the `Reuse Compliance` workflow all files must contain an acceptable license tag. This add the necessary tags to all of the existing examples and demos so that we can implement the reuse compliance workflow for future contributions. Signed-off-by: Winford <winford@object.stream>
Adds a copy of the reuse compliance check from AtomVM workflows, and adds coppies of licenses to `LICENSES` directory. Signed-off-by: Winford <winford@object.stream>
…format to IO.puts. Getting crash
You will need to run:
from your branch to catch up with the last merge. And then go ahead and flatten you changes into a single commit. We don't need all of your changes and revisions in the repo history. It looks like you will need to add a copyright notice and license tag to the header of some of your files. You can look in the other examples (they have all been updated in the last merge) to see what the copyright and license tags should look like. Make sure you give yourself the copyright credit and use the current year. You can see the list of files that need updating here: I would wait until you are done adding the license info, and all checks pass before you flatten you changes, that will save you the trouble of doing it several times. |
The
APIRequest
AtomVM application connects to a wifi, and when the connection is established retrieves and prints on console a GET request over https.