-
-
Notifications
You must be signed in to change notification settings - Fork 23
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
feat: add development flag to scripts/configure.py #511
Conversation
LGTM |
display=flags.display, audiodetector=flags.audiodetector, | ||
firmware=flags.firmware, password=flags.password, | ||
restart_updater=flags.restart_updater) | ||
if not result: | ||
sys.exit(1) |
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.
note to self: test that updater successfully restarts after this change, cuz it uses subprocess.Popen
to call this and may care about exit status.
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.
Subprocess only exceptions on non-zero exit when check_call()
/check_output()
are used: https://docs.python.org/3/library/subprocess.html#exceptions
since our use of the Popen constructor is basically throwaway ("send the reboot command and return without waiting"), this seems safe. this also functioned correctly when I tested.
@@ -11,7 +11,7 @@ Jinja2==3.1.2 | |||
loguru==0.6.0 | |||
mypy==1.0.0 | |||
netifaces==0.11.0 | |||
numpy==1.21.6 | |||
numpy |
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.
note to self: numpy is imported to speed up rendering on the TFT display, as recommended by https://github.com/adafruit/Adafruit_CircuitPython_RGB_Display . it's only used a small handful of places here and without a version bound; validate all potential versions of numpy haven't substantially changed the functions used here: https://github.com/adafruit/Adafruit_CircuitPython_RGB_Display/blob/main/adafruit_rgb_display/rgb.py and also that the TFT display works well.
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.
The only code that uses numpy in the Adafruit library is here: https://github.com/adafruit/Adafruit_CircuitPython_RGB_Display/blob/8cfb0db7d36eb46ef1e2a575b03f5e630bfc2c94/adafruit_rgb_display/rgb.py#L68
The functions & methods in question are:
numpy.array()
numpy.dstack().flatten().tolist()
On first brush with the documentation, these seem well supported (indeed, foundational) from 1.21 thru 1.26, the latest numpy release.
This still probably deserves a run on a box with a TFT display to validate.
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 validated this functions on a box with a TFT display.
lastly, I manually merged #524 into a local branch and validated that mypy and pylint (and all other tests, though those felt ancillary) were happy with these changes. |
I think the real thing to test here since configure.py was modified is can we update from this version to something else |
in terms of configure.py, I've tested:
|
This PR adds a simple
--development
flag toscripts/configure.py
. At present, it simply disregards the platform checking; I suspect it could be helpful to use elsewhere too though.Also,
.gitignore
: we now have errantnode_modules/
floating around cuz ofwebapp_2
development, and also my.swp
files fromvi
.