adafruit / Adafruit_CircuitPython_MiniMQTT

MQTT Client Library for CircuitPython
Other
73 stars 50 forks source link

Settings dot toml #162

Closed BlitzCityDIY closed 1 year ago

BlitzCityDIY commented 1 year ago

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 commented 1 year ago

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

github-actions[bot] commented 1 year ago

👋 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 commented 1 year ago

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 commented 1 year ago

@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?

tekktrik commented 1 year ago

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

FoamyGuy commented 1 year ago

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 commented 1 year ago

excellent, thank you @FoamyGuy !

FoamyGuy commented 1 year ago

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!

tekktrik commented 1 year ago

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