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

Improve deprecations #6097

Merged
merged 2 commits into from Jul 11, 2018

Conversation

Projects
None yet
3 participants
@illicitonion
Copy link
Contributor

illicitonion commented Jul 11, 2018

  • Only warn about the 'source' argument if it's actually being paid
    attention to by the Target, rather than if it's being ignored by the
    target.
  • Use an eager empty fileset, rather than a lazy one, if sources is an
    empty collection literal in a BUILD file - this means we don't get a
    deprecation in this case, as we shouldn't.
Improve deprecations
* Only warn about the 'source' argument if it's actually being paid
  attention to by the Target, rather than if it's being ignored by the
  target.
* Use an eager empty fileset, rather than a lazy one, if sources is an
  empty collection literal in a BUILD file - this means we don't get a
  deprecation in this case, as we shouldn't.

@illicitonion illicitonion requested review from stuhood and cosmicexplorer Jul 11, 2018

'The source argument will stop being populated -').format(target.type_alias),
)
unknown_args.pop('source')
if 'source' in kwargs and 'source' not in unknown_args and 'source' not in ignore_params:

This comment has been minimized.

@cosmicexplorer

cosmicexplorer Jul 11, 2018

Contributor

I'm having real difficulty parsing this line (could just be the time) -- it seems like 'source' in kwargs and 'source' not in unknown_args and 'source' not in ignore_params is always going to be false, given the definition of unknown_args just above? Not trying to nitpick -- if there's something I'm missing let me know, I just feel like a very short comment describing the condition being checked here could be helpful in the future (since the point of this diff is to refactor that condition).

This comment has been minimized.

@illicitonion

illicitonion Jul 11, 2018

Contributor

Ugh, you're right, this isn't ever actually going to be hit. I'm just going to remove the deprecation warning, and cross my fingers...

@cosmicexplorer cosmicexplorer merged commit 450de70 into pantsbuild:master Jul 11, 2018

1 check passed

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

@illicitonion illicitonion deleted the twitter:dwagnerhall/sourcesdeprecations branch Aug 15, 2018

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