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

Rename environment.get_value_string() to get_value_raw(). #4132

Draft
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

letitz
Copy link
Collaborator

@letitz letitz commented Jul 31, 2024

This is a prelude to fixing type errors in environment.py.

One of the big sources of type errors so far has been the fact that environment.get_value() is ~untyped. Many places in the codebase assume that it returns e.g. a string, or an int. We can introduce get_value_int() for example for ints that checks at runtime that the environment variable evaluated to an int, and use that instead where applicable to appease the
type checker (and surface errors earlier at runtime).

There is a need for a get_value_string() that tries to evaluate environment variable contents, i.e. supports quoted values. Many variables are expected to be strings, yet the pervasive use of environment.get_value() and environment.set_value() means it is hard to tell whether they are sometimes quoted or not.

This PR renames the current get_value_string() function to make space for a future get_value_string() that evaluates variables.

Copy link
Collaborator

@jonathanmetzman jonathanmetzman left a comment

Choose a reason for hiding this comment

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

lgtm
Happy to land this when you are.

@letitz
Copy link
Collaborator Author

letitz commented Sep 17, 2024

Thanks for the review! I've rebased and made the type annotation more precise, to avoid errors like:

foo = environment.get_value_raw('foo', '')
bar = foo.split(',')   # type checker complains `foo` might be None

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.

2 participants