fix: remove bytes comparison in Path.convert to avoid BytesWarning#3249
fix: remove bytes comparison in Path.convert to avoid BytesWarning#3249yunior123 wants to merge 1 commit intopallets:mainfrom
Conversation
BytesWarning Path.convert compared rv against (b"-", "-") which triggers BytesWarning under python -bb since rv is always str. The bytes check was dead code as the value type is str | PathLike[str]. Fixes pallets#2877
There was a problem hiding this comment.
Pull request overview
Removes a bytes vs str comparison in click.types.Path.convert to prevent BytesWarning when running Python with -bb -Werror, and adds a regression test around allow_dash.
Changes:
- Simplify the
allow_dashcheck inPath.convertto avoid comparing againstb"-". - Add a new test intended to ensure no
BytesWarningoccurs whenallow_dash=True.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
src/click/types.py |
Replaces rv in (b"-", "-") with a string-only dash check to avoid BytesWarning. |
tests/test_arguments.py |
Adds a regression test around Path(allow_dash=True) and warnings behavior. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| def test_path_allow_dash_no_bytes_warning(runner): | ||
| """Path with allow_dash should not compare str against bytes.""" | ||
| import warnings | ||
|
|
||
| @click.command() | ||
| @click.argument("input", type=click.Path(allow_dash=True)) | ||
| def foo(input): | ||
| click.echo(input) | ||
|
|
||
| with warnings.catch_warnings(): | ||
| warnings.simplefilter("error", BytesWarning) | ||
| result = runner.invoke(foo, ["-"]) | ||
| assert result.exit_code == 0 |
There was a problem hiding this comment.
This test likely won't catch the original regression in normal test runs. BytesWarning for bytes/str comparisons is only emitted when Python is started with -b/-bb (as in the issue report); setting warnings.simplefilter("error", BytesWarning) doesn't enable emission if sys.flags.bytes_warning == 0. Consider asserting the behavior via a subprocess started with sys.executable -bb -Werror (similar to tests/test_imports.py), or otherwise ensure the test only runs / is meaningful when sys.flags.bytes_warning is enabled.
| rv = value | ||
|
|
||
| is_dash = self.file_okay and self.allow_dash and rv in (b"-", "-") | ||
| is_dash = self.file_okay and self.allow_dash and rv == "-" |
There was a problem hiding this comment.
rv == "-" avoids the str vs bytes tuple membership warning, but if value is ever a bytes path (e.g. passed programmatically or via CliRunner.invoke with bytes args), this comparison will still emit BytesWarning under -bb. Using os.fsdecode(rv) == "-" for the dash check would avoid any bytes/str comparison and matches the approach used in open_stream for handling Path("-").
| is_dash = self.file_okay and self.allow_dash and rv == "-" | |
| is_dash = self.file_okay and self.allow_dash and os.fsdecode(rv) == "-" |
Fixes #2877. Remove dead b"-" from the dash check in Path.convert since the value is always str | PathLike[str], never bytes. Running with python -bb -Werror no longer raises BytesWarning.