Fix encoding warnings with PEP 597 enabled

[danyeaw]

Pull Request Check List

Resolves: #8892

• Added tests for changed code.
• Updated documentation for changed code.

This PR fixes Encoding Warnings with PEP 597 warnings enabled and enables the warnings in CI. There are also a few warnings with poetry-core and cleo, and I can submit PRs for those repos as well.

[radoering]

pre-commit.ci autofix

[radoering]

There are still some warnings we should address.

[danyeaw]

We are down to 1 warning:

tests/console/commands/test_init.py::test_respect_prefer_active_on_init[True-1.1]
  /home/dan/Projects/poetry/tests/console/commands/test_init.py:1091: EncodingWarning: 'encoding' argument not specified.
    result: str = orig_check_output(cmd, *_, **__)

I'm not sure what to do about that one since it is a mocked out version of check_output.

[danyeaw]

Actually 1 more as well:

tests/vcs/git/test_system.py::TestSystemGit::test_clone_success
  /home/dan/Projects/poetry/src/poetry/vcs/git/system.py:43: EncodingWarning: 'encoding' argument not specified.
    subprocess.check_call(

I'm not sure how to apply encodings to a check_call.

[radoering]

I'm not sure what to do about that one since it is a mocked out version of check_output.

The warning comes from calling the original method so I suppose at least one check_output() does not specify an encoding. Did you check that each check_output in the code base specifies an encoding? (Alternatively, you can catch the warning and re-emit it with stacklevel=2 to find the caller.)

I'm not sure how to apply encodings to a check_call.

Probably, the same as for the other methods. According to the docs it supports the same kwargs as Popen.

[danyeaw]

Hi @radoering, I rebased this on top of the main branch. Unfortunately mypy gives a Unexpected keyword argument "encoding" for "check_call".

[radoering]

If check_call supports this keyword and mypy thinks it does not, this is probably a mypy/typeshed bug and we should just add a type: ignore (and even better additionally create an issue/PR at https://github.com/python/typeshed).

What surprises me a little is that I no longer see any encoding warnings in the test output in the CI. Am I overlooking something? Does enabling the default encoding warning does not work anymore after the rebase?

[danyeaw]

Hi @radoering, thanks again for the feedback. I created python/typeshed#12084 and added a type ignore.

[dimbleby]

https://docs.github.com/en/enterprise-cloud@latest/actions/using-workflows/reusing-workflows#limitations

Any environment variables set in an env context defined at the workflow level in the caller workflow are not propagated to the called workflow

#9534

[radoering]

@dimbleby Thanks for finding that out.

@danyeaw Can you adopt the change from #9534 to see which warnings still need to be addressed, please?

[danyeaw]

Is this one ready to merge now?

[radoering]

Is this one ready to merge now?

We cannot do much about warnings in site-packages but there are still warnings that come from code in this repo. We should fix all warnings that come directly from this repo before enabling encoding warnings in the main branch. Please also take a look at the test runs for the different operating systems. Some warnings only show up on one of them.

[dimbleby]

@danyeaw are you intending to pick off the remaining warnings?

If not then I suggest that this is a case where three quarters of a fix is better than no fix at all, might as well merge anyway.

[danyeaw]

@dimbleby, I'm probably done working on this one to be honest. I think this gets things most of the way there, and eventually Poetry could adopt the Ruff PLW1514 rule https://docs.astral.sh/ruff/rules/unspecified-encoding/, and remove the encoding warning environmental variable.

[radoering]

I will take a look at it next week and probably either deactivate the warning only merging the fixes or fix the remaining warnings.

[radoering]

I think all warnings in the poetry code base are fixed now. In other words, the only existing warnings are in site packages.

@dimbleby Can you please take a look at the last commit? If you do not spot a bug I will merge this.

[dimbleby]

Looks like that should resolve the warnings, without changing the behaviour - right?

Seems sensible. I find it more than confusing to work out what the "correct" values are supposed to be! But I don't recall any recent issues relating to funky locales and non-ascii paths - so let's not disturb the balance.

[danyeaw]

Hi @radoering, thanks for the teamwork on this.

[github-actions]

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.