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

👷 Set PYTHONWARNDEFAULTENCODING in GitHub Workflow #93

Merged
merged 1 commit into from
Jul 14, 2023

Conversation

kkirsche
Copy link
Contributor

Fix #91

This merge request sets the PYTHONWARNDEFAULTENCODING variable in the GitHub workflow per the documentation at https://docs.github.com/en/actions/learn-github-actions/variables.

I've chosen to set PYTHONWARNDEFAULTENCODING to the value true to ensure it is clear that this is enabling the feature. With that said, https://docs.python.org/3/library/io.html#opt-in-encodingwarning does not indicate that there is a value of the environment variable that would disable this, so I understand if a different value would be preferred.

Fix pydantic#91 

This merge request sets the `PYTHONWARNDEFAULTENCODING` variable in the GitHub workflow per the documentation at https://docs.github.com/en/actions/learn-github-actions/variables.

I've chosen to set `PYTHONWARNDEFAULTENCODING` to the value `true` to ensure it is clear that this is enabling the feature. With that said, https://docs.python.org/3/library/io.html#opt-in-encodingwarning does not indicate that there is a value of the environment variable that would disable this, so I understand if a different value would be preferred.
@kkirsche kkirsche changed the title Set PYTHONWARNDEFAULTENCODING Set PYTHONWARNDEFAULTENCODING in GitHub Workflow Jul 14, 2023
@Kludex Kludex changed the title Set PYTHONWARNDEFAULTENCODING in GitHub Workflow 👷 Set PYTHONWARNDEFAULTENCODING in GitHub Workflow Jul 14, 2023
Copy link
Member

@Kludex Kludex left a comment

Choose a reason for hiding this comment

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

Thank you so much! This kind of PR helps me a lot. 🙇

@Kludex Kludex merged commit 98a3666 into pydantic:main Jul 14, 2023
7 checks passed
@kkirsche kkirsche deleted the patch-1 branch July 14, 2023 11:16
@kkirsche
Copy link
Contributor Author

Happy to help out 🙂 looks like there were two warnings in the tests, so shouldn't be too bad to get those addressed.

=============================== warnings summary ===============================
tests/integration/test_cli.py::test_command_line
tests/integration/test_cli.py::test_command_line
  /home/runner/work/bump-pydantic/bump-pydantic/bump_pydantic/codemods/mypy_visitor.py:34: EncodingWarning: 'encoding' argument not specified
    result = build(files, opt, stdout=sys.stdout, stderr=sys.stderr)

tests/integration/test_cli.py::test_command_line
  /home/runner/work/bump-pydantic/bump-pydantic/bump_pydantic/main.py:83: EncodingWarning: 'encoding' argument not specified
    log_fp = log_file.open("a+")

-- Docs: https://docs.pytest.org/en/stable/how-to/capture-warnings.html

@Kludex
Copy link
Member

Kludex commented Jul 14, 2023

Happy to help out 🙂 looks like there were two warnings in the tests, so shouldn't be too bad to get those addressed.

=============================== warnings summary ===============================
tests/integration/test_cli.py::test_command_line
tests/integration/test_cli.py::test_command_line
  /home/runner/work/bump-pydantic/bump-pydantic/bump_pydantic/codemods/mypy_visitor.py:34: EncodingWarning: 'encoding' argument not specified
    result = build(files, opt, stdout=sys.stdout, stderr=sys.stderr)

tests/integration/test_cli.py::test_command_line
  /home/runner/work/bump-pydantic/bump-pydantic/bump_pydantic/main.py:83: EncodingWarning: 'encoding' argument not specified
    log_fp = log_file.open("a+")

-- Docs: https://docs.pytest.org/en/stable/how-to/capture-warnings.html

Wanna help? 👀 🙏

Oh, and it looks like we want to fail tests on warning!

@kkirsche
Copy link
Contributor Author

Happy to, it'll be later today before I can get to that but shouldn't be a problem at all. I'll need to read the docs on failing on warnings but I'd be surprised if pytest made that a difficult thing to configure. It seems like it'd be something many teams would want.

@Kludex
Copy link
Member

Kludex commented Jul 14, 2023

I think those 2 lines need to be added for the tests to fail on warning: https://github.com/encode/starlette/blob/844220819f1cfcf2726d1628dd4ea597182d6f3e/pyproject.toml#L77-L79

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.

Add PYTHONWARNDEFAULTENCODING to pipeline
2 participants