Skip to content

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

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

tomashco
Copy link

The APIRequest AtomVM application connects to a wifi, and when the connection is established retrieves and prints on console a GET request over https.

Copy link
Collaborator

@UncleGrumpy UncleGrumpy left a 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).

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",
Copy link
Collaborator

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 = [
Copy link
Collaborator

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!

@@ -10,7 +10,7 @@ defmodule Blinky.MixProject do
deps: deps(),
atomvm: [
start: Blinky,
flash_offset: 0x210000
Copy link
Collaborator

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.

@@ -10,7 +10,7 @@ defmodule HelloWorld.MixProject do
deps: deps(),
atomvm: [
start: HelloWorld,
flash_offset: 0x210000
Copy link
Collaborator

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.

#
# This file is part of AtomVM.
#
# Copyright 2018 Davide Bettio <davide@uninstall.it>
Copy link
Collaborator

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 ;-)

Copy link
Collaborator

@UncleGrumpy UncleGrumpy left a 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.
Copy link
Collaborator

@UncleGrumpy UncleGrumpy Feb 18, 2025

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. ;-)

@UncleGrumpy
Copy link
Collaborator

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.

UncleGrumpy and others added 4 commits February 18, 2025 22:08
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>
@UncleGrumpy
Copy link
Collaborator

UncleGrumpy commented Feb 19, 2025

You will need to run:

git pull --rebase

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:
https://github.com/atomvm/atomvm_examples/actions/runs/13401774653/job/37436670080?pr=21#step:4:6

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants