Skip to content

Refactor and Document check_env.check()#1826

Open
bmos wants to merge 15 commits into
move-coop:mainfrom
bmos:check_env
Open

Refactor and Document check_env.check()#1826
bmos wants to merge 15 commits into
move-coop:mainfrom
bmos:check_env

Conversation

@bmos
Copy link
Copy Markdown
Collaborator

@bmos bmos commented Apr 10, 2026

What is this change?

  • Adjust a few nonstandard imports of check directly rather than via importing check_env.
  • Add comprehensive documentation to check_env.check().
  • Convert optional parameter to keyword-only.
  • Rename field parameter to value.
  • Improve tests for check_env.check
  • Add deprecation handler to avoid optional and field breaking changes.

Considerations for discussion

  • Despite being a core function used in many connectors, check_env.check() was minimally documented.
  • Allowing users to provide boolean values for positional parameters is not recommended as their intention can be confusing.

How to test the changes (if needed)

  • CI tests should be adequate

Breaking Changes

Breaking changes are changes to our public API which may require existing users to change their code. If there are no breaking changes, any existing parsons user should not need to do anything after updating their parsons version.

Does this PR introduce breaking changes?
  • label: Breaking change — This PR introduces one or more breaking changes.
  • label: Non-breaking change — This PR does not introduce one or more breaking changes.

@github-actions
Copy link
Copy Markdown

ghost commented Apr 10, 2026

Coverage report

Click to see where and how coverage changed

FileStatementsMissingCoverageCoverage
(new stmts)
Lines missing
  parsons/aws
  lambda_distribute.py 205
  parsons/braintree
  braintree.py
  parsons/notifications
  slack.py 128
  parsons/redash
  redash.py
  parsons/utilities
  check_env.py 78-85, 88-95
Project Total  

This report was generated by python-coverage-comment-action

@bmos bmos marked this pull request as ready for review April 10, 2026 23:28
@bmos bmos marked this pull request as draft April 15, 2026 16:27
@bmos bmos marked this pull request as ready for review April 15, 2026 18:00

def check(env: str, field: str | None, optional: bool | None = False) -> str | None:

@overload
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

What are these @overload decorators doing?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Overload is a type hinting feature.
It tells static type checkers that if you provide a certain type as an input you get a certain type as an output.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Gotcha. Why are there three of them? It feels more verbose than is ideal (but maybe if we can use a comment to signal to the reader that these are type hints with no impact on the code itself, the verbosity is less of an issue)

Copy link
Copy Markdown
Collaborator Author

@bmos bmos May 22, 2026

Choose a reason for hiding this comment

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

That's just how the syntax works, unfortunately.
That's why overload is only used situationally. I think it's worth it here since check is used in a ton of connectors so having accurate typing is extra useful.

Copy link
Copy Markdown
Collaborator Author

@bmos bmos May 22, 2026

Choose a reason for hiding this comment

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

Happy to chat through how this works sometime, I just keep being busy on Thursdays 😅
It's a very helpful feature.

Copy link
Copy Markdown
Collaborator

@shaunagm shaunagm May 22, 2026

Choose a reason for hiding this comment

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

You've got some other PRs I could review too so yeah let's schedule a call.

Name of environment variable to check.
value:
If provided, ignore environment variable and return this.
opt:
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Where is the opt parameter coming from? I get that it's being listed as deprecated, but I'm not seeing where it's used at all.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

optional (the former 3rd positional argument) is now a keyword-only argument. So I added "opt" as the 3rd positional argument with the deprecation warning. That way it's not a breaking change if people try to provide the optional parameter as a positional argument.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Gotcha. What's your reasoning on moving it from a 3rd positional argument to a keyword-only argument?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

non-breaking-change Status - Indicates that the code in this PR does not have any breaking changes. python Pull requests that update Python code testing Work type - writing or changing code tests for core Parsons features or Parsons connectors

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants