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

fix: always use parameter UTF-8 fallback #1468

Merged
merged 2 commits into from Mar 6, 2020
Merged

Conversation

@muggenhor
Copy link
Contributor

muggenhor commented Feb 10, 2020

Even when the filesystem encoding and sys.argv encoding are the same we
should still fall back to trying UTF-8 for decoding command line
arguments.

@davidism

This comment has been minimized.

Copy link
Member

davidism commented Feb 24, 2020

This seems ok, but I can't tell what it might affect. Can you add a test case?

click/types.py Show resolved Hide resolved
muggenhor added 2 commits Mar 2, 2020
Even when the filesystem encoding and sys.argv encoding are the same we
should still fall back to trying UTF-8 for decoding command line
arguments.
@muggenhor muggenhor force-pushed the muggenhor:fix/utf-8-compat branch from cfe9117 to f6710e5 Mar 2, 2020
@muggenhor muggenhor changed the base branch from master to 7.x Mar 2, 2020
@muggenhor

This comment has been minimized.

Copy link
Contributor Author

muggenhor commented Mar 2, 2020

I've made the test case commit come first. That way it's easier to verify that it fails without the fix commit.

For reference, the monkeypatched values I inserted are those that Python 2 and 3 respectively set when starting with all of the LC_* and LANG* environments cleared. This happens only at Python startup however, so I changing these variables inside the test case itself no longer has an effect, hence the monkeypatch approach.

@davidism davidism added this to the 7.1 milestone Mar 6, 2020
@davidism

This comment has been minimized.

Copy link
Member

davidism commented Mar 6, 2020

Just leaving a note for myself in case I need to revisit this later. It seems like .decode("utf-8", "replace") is already used as a fallback, it just wasn't applied consistently. Since this seems to improve consistency, I'm merging it.

Thanks for working on this!

@davidism davidism merged commit 08f8211 into pallets:7.x Mar 6, 2020
13 checks passed
13 checks passed
Tests Build #20200302.2 succeeded
Details
Tests (Job Docs) Job Docs succeeded
Details
Tests (Job PyPy 3 Linux) Job PyPy 3 Linux succeeded
Details
Tests (Job Python 2.7 Linux) Job Python 2.7 Linux succeeded
Details
Tests (Job Python 2.7 Windows) Job Python 2.7 Windows succeeded
Details
Tests (Job Python 3.5 Linux) Job Python 3.5 Linux succeeded
Details
Tests (Job Python 3.6 Linux) Job Python 3.6 Linux succeeded
Details
Tests (Job Python 3.7 Linux) Job Python 3.7 Linux succeeded
Details
Tests (Job Python 3.7 Mac) Job Python 3.7 Mac succeeded
Details
Tests (Job Python 3.7 Windows) Job Python 3.7 Windows succeeded
Details
Tests (Job Python 3.8 Linux) Job Python 3.8 Linux succeeded
Details
Tests (Job Python 3.8 Mac) Job Python 3.8 Mac succeeded
Details
Tests (Job Python 3.8 Windows) Job Python 3.8 Windows succeeded
Details
sirosen added a commit to globus/globus-cli that referenced this pull request Mar 11, 2020
In pallets/click#1468 , string parameter types (the default) added a
catchall case in which a value which fails to decode is run though
`decode('utf-8', 'replace')`. This succeeds even on things like
latin-1 chars which can't decode into utf-8 . As a result, our test
case which relied on the value being passed through verbatim doesn't
work anymore. However, the case which uses our custom param type (the
mkdir test) still fails as expected.

We may still be able to and want to test our failure modes around
invalid UTF8 bytes like this, but for the purposes of being click 7.1
compatible, just remove the failing test.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants
You can’t perform that action at this time.