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

Simplify PathGlobs python datatype #5915

Merged
merged 2 commits into from Jun 6, 2018

Conversation

Projects
None yet
2 participants
@illicitonion
Copy link
Contributor

illicitonion commented Jun 5, 2018

It's generally empty

@stuhood

This comment has been minimized.

Copy link
Member

stuhood commented Jun 5, 2018

How would we feel about removing it entirely? It's sugar at this point, and would likely just be used incorrectly in 99% of cases.

That would also allow you to remove the PathGlobs.create method in favor of just overriding __new__ to default excludes.

@stuhood

stuhood approved these changes Jun 5, 2018

Copy link
Member

stuhood left a comment

Looks good... think you can go a bit further though.

@@ -57,15 +57,15 @@ def with_match_error_behavior(self, glob_match_error_behavior):
glob_match_error_behavior=glob_match_error_behavior)

@staticmethod
def create(relative_to, include, exclude=tuple(), glob_match_error_behavior=None):
def create(include, exclude=tuple(), glob_match_error_behavior=None, relative_to=''):

This comment has been minimized.

@stuhood

stuhood Jun 5, 2018

Member

Now that relative_to is not a required argument, I believe that you could just switch this to an override of __new__ rather than a factory method? ie, remove PathGlobs.create(..) in favor of just PathGlobs(..).

@illicitonion illicitonion changed the title PathGlobs.create's relative_to arg is optional Simplify PathGlobs python datatype Jun 6, 2018

@illicitonion illicitonion merged commit 4f5a756 into pantsbuild:master Jun 6, 2018

1 check passed

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

@illicitonion illicitonion deleted the twitter:dwagnerhall/lazyfileset/globsrelativeto branch Jun 6, 2018

@stuhood

This comment has been minimized.

Copy link
Member

stuhood commented Jun 6, 2018

Awesome.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment