Skip to content

getenv: Make os.getenv() show a better error #10422

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

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

jepler
Copy link

@jepler jepler commented Jun 14, 2025

.. when an associated value is not a quoted string. This includes some cases where it would previously return an integer, a CPython incompatibility.

However, it's an incompatible behavior change with circuitpython since previously a number would be returned.

Closes: #9113

.. when an associated value is not a quoted string.
This includes some cases where it would previously return an
integer, a CPython incompatibility.

However, it's an incompatible behavior change with circuitpython
since previously a number would be returned.

Closes: adafruit#9113

Signed-off-by: Jeff Epler <jepler@gmail.com>
@Neradoc
Copy link

Neradoc commented Jun 14, 2025

If we are completely disallowing ints then it's a breaking change that we need to document clearly with the mention that we only accept strings in our subset of toml (and that they need double quotes).
https://docs.circuitpython.org/en/latest/docs/environment.html#details-of-the-toml-language-subset

• The supported data types are string and integer
...
• Only integers supported by strtol. (no 0o, no 0b, no underscores 1_000, 011 is 9, not 11)

Alternatively we could still allow reading ints but return them as strings (and have a better error) but that could be confusing.

Also, it looks like this preserves common_hal_os_getenv_int, but does that mean that the user code can't read some of the environment variables used by Circuitpython ?

CIRCUITPY_WEB_API_PORT=8000
CIRCUITPY_DISPLAY_WIDTH=640
CIRCUITPY_DISPLAY_COLOR_DEPTH=1

@jepler
Copy link
Author

jepler commented Jun 14, 2025

I have no HW so I'm not able to test on HW, but under this PR the intent is that internal routines that get integers will continue to work as before.

It is only intended to change the os.getenv API so that it can no longer return integers (cpython compatibility) and will issue a better error message in the case that a non-quoted value is encountered with a key passed to os.getenv.

I'm not sure whether I'm in favor of this change myself, because as you say it ought to go through deprecation and does remove the ability to read integer-syntax values from settings.toml into python code without adding it back in some hidden way. It's just that I was still being copied on comments on #9113 and noticed a recent one go by. I decided to check into the coding required to implement CPython compatibility and this is what I came up with. But I'm not actually itching to see it merged, to be honest.

@dhalbert dhalbert marked this pull request as draft June 14, 2025 18:15
@dhalbert
Copy link
Collaborator

I'm making this draft so it won't get merged by accident. I'll discuss this further in #9113.

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.

os.getenv("A_NUMBER") can return an int
3 participants