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

enum cleanup #7269

Merged

Conversation

Projects
None yet
3 participants
@cosmicexplorer
Copy link
Contributor

commented Feb 20, 2019

Problem

Resolves #7232, resolves #7248, and addresses #7249 (comment).

Solution

  • Remove enum defaults and the .create() method with complicated argument handling.
  • Allow enums to be == compared as long as they're the same type.
  • Allow accessing enum instances with class attributes.

Result

enums are nicer!

@baroquebobcat
Copy link
Contributor

left a comment

Looks pretty good to me! Just one comment about a error message.

Show resolved Hide resolved src/python/pants/util/objects.py Outdated
@illicitonion
Copy link
Contributor

left a comment

Looks good, thanks for picking this up! There are a few more things I think we can simplify away, too :)

Show resolved Hide resolved src/python/pants/backend/native/subsystems/native_build_step.py
Show resolved Hide resolved src/python/pants/option/custom_types.py Outdated
Show resolved Hide resolved src/python/pants/util/objects.py Outdated
Show resolved Hide resolved src/python/pants/util/objects.py Outdated
Show resolved Hide resolved src/python/pants/util/objects.py Outdated
Show resolved Hide resolved src/python/pants/util/objects.py
Show resolved Hide resolved tests/python/pants_test/util/test_objects.py
@cosmicexplorer

This comment has been minimized.

Copy link
Contributor Author

commented Feb 22, 2019

Going to address a few comments from @illicitonion above before merging, two of which should have their own PRs.

@cosmicexplorer

This comment has been minimized.

Copy link
Contributor Author

commented Mar 1, 2019

#7233 and #7298 remain outstanding issues, but I have otherwise been able to address all of the comments above!

@cosmicexplorer cosmicexplorer force-pushed the cosmicexplorer:more-enum-improvements branch from 465d736 to d94a053 Mar 1, 2019

cosmicexplorer added a commit that referenced this pull request Mar 1, 2019

add checker for accidental constants in boolean operators (#7273)
### Problem

It's possible to accidentally write something like `None or 'default_value'`, which is often an error indicating that the original value is dropped. One example is in 13b8505 for #7269 just now.

### Solution

- Add a checker for `and`/`or` expressions which ensures that the left-hand side is probably not a constant, and ensures that the right-hand side is probably not a constant for `and` expressions.

### Result

One possible class of error that I just made is impossible.

cosmicexplorer added some commits Feb 20, 2019

@cosmicexplorer cosmicexplorer force-pushed the cosmicexplorer:more-enum-improvements branch from adf5262 to 112ca3e Mar 1, 2019

cosmicexplorer added some commits Mar 1, 2019

@cosmicexplorer cosmicexplorer merged commit af19119 into pantsbuild:master Mar 2, 2019

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
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.