-
Notifications
You must be signed in to change notification settings - Fork 359
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
Add load_settings_toml #949
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.
# | ||
# SPDX-License-Identifier: MIT | ||
[pytest] | ||
pythonpath = src |
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.
needed so pytest knew where the files were. I see there are other tests, but that CI doesn't tun them
@@ -99,6 +99,7 @@ | |||
"pyftdi>=0.40.0", | |||
"adafruit-circuitpython-typing", | |||
"sysv_ipc>=1.1.0;sys_platform=='linux' and platform_machine!='mips'", | |||
"toml>=0.10.2;python_version<'3.11'", |
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.
tomllib
added in 3.11
def load_settings_toml(*, return_toml=False): | ||
"""Load values from settings.toml into os.environ, so that os.getenv returns them.""" | ||
if not os.path.isfile("settings.toml"): | ||
raise FileNotFoundError("settings.toml not cound in current directory.") |
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.
File not found
try: | ||
settings = tomllib.load(toml_file) | ||
except tomllib.TOMLDecodeError as e: | ||
raise tomllib.TOMLDecodeError("Error with settings.toml file.") from e |
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.
Invalid file
invalid_types.add(type(value).__name__) | ||
if invalid_types: | ||
invalid_types_string = ", ".join(invalid_types) | ||
raise ValueError( |
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.
invalid types
|
||
invalid_types = set() | ||
for key, value in settings.items(): | ||
if not isinstance(value, (bool, int, float, str)): |
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.
these all get converted into strings (as they would be in the environment)
src/adafruit_blinka/__init__.py
Outdated
os.environ[key] = str(value) | ||
print(f" - {key} added") | ||
|
||
if return_toml: |
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.
Added for testing, but could also be helpful, it's like getting secrets back
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 think it's okay to just return settings all the time instead of make it conditional with an argument. In the majority of cases people will have copied code that just calls the function and ignores the result.
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 agree, no reason to control whether or not to return, removes an arg, and reduces code size. settings
will get gc'd the same way regardless of whether or not it is returned, if the caller does not assign it to anything.
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.
Sounds good
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.
Fixed here: 046217c
src/adafruit_blinka/__init__.py
Outdated
os.environ[key] = str(value) | ||
print(f" - {key} added") | ||
|
||
if return_toml: |
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 agree, no reason to control whether or not to return, removes an arg, and reduces code size. settings
will get gc'd the same way regardless of whether or not it is returned, if the caller does not assign it to anything.
@FoamyGuy are you opening up another PR to change the minimum pay version? |
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.
Thank you, this looks good to me. Tested successfully in the REPL on a pi5
No description provided.