Skip to content
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

Settings dot toml #162

Merged
merged 12 commits into from
Jul 7, 2023
Merged

Settings dot toml #162

merged 12 commits into from
Jul 7, 2023

Conversation

BlitzCityDIY
Copy link
Contributor

hihi @brentru - I updated the examples that are in the MiniMQTT guide to use settings.toml. Had to change up the WiFi connection method for ESP32SPI since adafruit_esp32spi_wifimanager assumes secrets is being used. Tested native networking with a QT Py ESP32-S2 and ESP32SPI with a Metro M7.

if you are good with how these are formatted I can update the other examples as well.

@brentru brentru self-requested a review April 25, 2023 15:40
@brentru
Copy link
Member

brentru commented Apr 25, 2023

@BlitzCityDIY When this has been updated and is ready. Under "Reviewers" please click the ♻️ next to my username that says "re-request review"

Retested examples using a Feather ESP32-S2 for native and a Metro M7 for esp32spi
@github-actions
Copy link

github-actions bot commented Jul 7, 2023

👋 Thanks for this pull request! Unfortunately, it looks like the automated continuous integration (CI) test(s) failed. These can be tricky to fix so we've written a guide on how to fix them locally. It has pages about running pre-commit locally and another about building the docs locally with sphinx. Thanks for contributing to CircuitPython! If you have more questions, feel free to join the Adafruit Discord and post in #circuitpython-dev.

@BlitzCityDIY
Copy link
Contributor Author

hihi @brentru - i went through and retested this PR and made some tweaks. all native networking examples were tested with a feather esp32-s2 and the esp32spi examples were tested with a metro m7. i'm at a loss though on the two black errors. i've run pre-commit and black locally with both windows command line and WSL and everything is passing but CI keeps flagging those two examples for black errors.

@brentru
Copy link
Member

brentru commented Jul 7, 2023

@tekktrik @FoamyGuy Not sure if you are the correct people to ping here. @BlitzCityDIY needs some help with the Black issues blocking this PR from being merged. Could you please take a look into this?

Copy link
Member

@brentru brentru left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code looks good. Updated to use the settings.toml and using f strings for the print statements is a welcome touch!

@tekktrik
Copy link
Member

tekktrik commented Jul 7, 2023

I'm unavailable this weekend but can take a look afterwards!

@FoamyGuy
Copy link
Contributor

FoamyGuy commented Jul 7, 2023

I think I ran across this recently somewhere else too. I figured it was something weird about my local environment, but perhaps it's a more widespread issue.

It seems that for some reason the Black formatting can have different results when it's running inside the actions container vs. when it runs locally.

This PR branch is passing pre-commit for me locally:
image

But failed in the actions check:

Error: black....................................................................Failed
- hook id: black
- files were modified by this hook

Error: reformatted examples/esp32spi/minimqtt_pub_sub_nonblocking_esp32spi.py
Error: reformatted examples/native_networking/minimqtt_pub_sub_blocking_native_networking.py

All done! ✨ 🍰 ✨
2 files reformatted, 21 files left unchanged.

It looks like the version specified in the pre-commit config was changed recently-ish (~2 months) but this PR branch perhaps was made before that, so it's config file is still on the old one. Then I guess when it runs in actions it's running under the "wrong" one, or a different one from main or something.

@tekktrik I had a look through the actions log and didn't find anywhere that it printed the version of black that it actually used. It printed versions for many other things, but I think not black. If possible / easy it might be nice if we can add black --version into the actions task somewhere so that it'll pipe that through into the logs to make it easier to figure out when anything like this difference result in local vs. remote that pops up.

@BlitzCityDIY
Copy link
Contributor Author

excellent, thank you @FoamyGuy !

@FoamyGuy
Copy link
Contributor

FoamyGuy commented Jul 7, 2023

It was indeed a difference of versions in black. It seems that when it runs in the actions container it implicitly merges main before the checks run. But when it's run locally there is nothing enforcing that behavior.

After I merged main I was able to get the same results as the actions check. The latest commits bring in main, and the changes that black formatter wanted.

Should be good to go if you want to merge this one now @brentru.

Thanks for working on this @BlitzCityDIY!

@brentru brentru merged commit 7b63446 into main Jul 7, 2023
2 checks passed
@brentru brentru deleted the settings_dot_toml branch July 7, 2023 18:32
adafruit-adabot added a commit to adafruit/Adafruit_CircuitPython_Bundle that referenced this pull request Jul 9, 2023
Updating https://github.com/adafruit/Adafruit_CircuitPython_ESP32SPI to 6.0.0 from 5.0.6:
  > Merge pull request adafruit/Adafruit_CircuitPython_ESP32SPI#167 from dhalbert/compatible-socket

Updating https://github.com/adafruit/Adafruit_CircuitPython_MiniMQTT to 7.4.0 from 7.3.2:
  > Merge pull request adafruit/Adafruit_CircuitPython_MiniMQTT#169 from vladak/loop_vs_timeout
  > Merge pull request adafruit/Adafruit_CircuitPython_MiniMQTT#162 from adafruit/settings_dot_toml
  > Update .pylintrc, fix jQuery for docs
  > Merge pull request adafruit/Adafruit_CircuitPython_MiniMQTT#167 from tekktrik/main
  > Run pre-commit
  > Update pre-commit hooks
  > Merge pull request adafruit/Adafruit_CircuitPython_MiniMQTT#164 from zbauman3/zane/cert-key-pair-example

Updating https://github.com/adafruit/Adafruit_CircuitPython_Requests to 2.0.0 from 1.14.1:
  > Merge pull request adafruit/Adafruit_CircuitPython_Requests#135 from FoamyGuy/remove_backward_compat

Updating https://github.com/adafruit/Adafruit_CircuitPython_WSGI to 2.0.0 from 1.1.16:
  > Merge pull request adafruit/Adafruit_CircuitPython_WSGI#20 from FoamyGuy/compatibility_refactor
  > Merge pull request adafruit/Adafruit_CircuitPython_WSGI#19 from tekktrik/dev/fix-packaging

Updating https://github.com/adafruit/Adafruit_CircuitPython_Bundle/circuitpython_library_list.md to NA from NA:
  > Updated download stats for the libraries
@tekktrik
Copy link
Member

I can add a step in the build workflow to read the .pre-commit-config.yaml file and print out relevant versions being used!

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.

4 participants