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

don't consider strings as collections for TypedCollection #7521

Conversation

Projects
None yet
5 participants
@cosmicexplorer
Copy link
Contributor

commented Apr 9, 2019

Problem

Python considers strings to be iterables of substrings, each containing a single character. Currently, TypedCollection(Exactly(text_type)) will accept not just collections of strings, but also just a single string, which is split into substrings.

Solution

  • Check for any string objects provided as input to a TypedCollection() field and avoid categorizing them as collections.
  • Use TypedCollection() fields for ExecuteProcessRequest!

Result

TypedCollection(Exactly(text_type)) doesn't allow the user to accidentally input a single string!

cosmicexplorer added some commits Apr 9, 2019

@cosmicexplorer cosmicexplorer force-pushed the cosmicexplorer:fix-typed-collection-string-iterable branch from c4e62e4 to 86a9e40 Apr 9, 2019

@stuhood

stuhood approved these changes Apr 9, 2019

@Eric-Arellano
Copy link
Contributor

left a comment

Great! This is one of the biggest gotchas in Python imo. Thanks for cleaning up.

Show resolved Hide resolved tests/python/pants_test/util/test_objects.py Outdated
('output_directories', tuple),
('env', TypedCollection(Exactly(text_type))),
('output_files', TypedCollection(Exactly(text_type))),
('output_directories', TypedCollection(Exactly(text_type))),

This comment has been minimized.

Copy link
@Eric-Arellano

Eric-Arellano Apr 9, 2019

Contributor

Great! This is one of the things I'd most like to see expanded with our datatype system, to allow complex types like Tuple[int, str], Dict[str, int], Optional[MyObject]. Maybe something we can achieve by merging with MyPy and type hints.

This comment has been minimized.

Copy link
@cosmicexplorer

cosmicexplorer Apr 9, 2019

Author Contributor

Ideally we just do something like #7074, and hope that dataclasses are something that easily extends to mypy, or we write a mypy plugin -- I'd prefer the first.

Show resolved Hide resolved src/python/pants/util/objects.py Outdated
Show resolved Hide resolved tests/python/pants_test/util/test_objects.py Outdated
@ity

ity approved these changes Apr 9, 2019

@Eric-Arellano
Copy link
Contributor

left a comment

Thank you!

@cosmicexplorer cosmicexplorer merged commit 61371c6 into pantsbuild:master Apr 9, 2019

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

cosmicexplorer added a commit that referenced this pull request Apr 11, 2019

allow all string (not binary!) types for ExecuteProcessRequest args (#…
…7529)

### Problem

#7521 introduced `TypedCollection` for `ExecuteProcessRequest` arguments, and the added type strictness exposed a place where we weren't explicitly converting a binary string to a text string, failing the cron job's python 2.7 shard: https://travis-ci.org/pantsbuild/pants/jobs/518207912.

### Solution

- Use `SubclassesOf(text_type)` instead of `Exactly(text_type)` to accept any string arguments (fixing the error in `rsc_compile.py` in the noted cron job).
- Explicitly convert the python executable path in the v2 pytest runner into a string.

### Result

The cron job passes again, and people can put in whatever string type they like to ExecuteProcessRequest, as long as it's a text string.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.