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

Drop old deprecated options / conditionals #7332

Merged
merged 7 commits into from Mar 13, 2019

Conversation

Projects
None yet
5 participants
@codealchemy
Copy link
Contributor

commented Mar 7, 2019

Similar to #7325, came across another option that appears to be safe to remove re: 1.14.0.dev2

@Eric-Arellano

This comment has been minimized.

Copy link
Contributor

commented Mar 7, 2019

We seem to have even more deprecations. Should these all be in one PR?

Found via rg -C 2 '\.dev' -g '*.py':

  • contrib/node/src/python/pants/contrib/node/subsystems/node_distribution.py
  • src/python/pants/backend/jvm/tasks/jvm_compile/jvm_compile.py
  • src/python/pants/backend/python/subsystems/python_setup.py
  • src/python/pants/backend/jvm/register.py
  • src/python/pants/build_graph/target.py
  • src/python/pants/build_graph/build_file_aliases.py

cc @stuhood

@stuhood

This comment has been minimized.

Copy link
Member

commented Mar 7, 2019

Feel free to nuke them all, yea!

I think that we must have lost eager validation of these registrations at some point, because I could have sworn that these used to fail eagerly.

EDIT: Opened #7335.

@codealchemy codealchemy changed the title Drop deprecated global option for pants fs event workers Drop deprecated options Mar 7, 2019

@codealchemy codealchemy changed the title Drop deprecated options Drop old deprecated options / conditionals Mar 7, 2019

return version if version.startswith('v') else 'v' + version

@classmethod
def _normalize_version(cls, version):

This comment has been minimized.

Copy link
@codealchemy

codealchemy Mar 7, 2019

Author Contributor

This was removed from the __init__ here, AFAICT it's not used elsewhere either so opted to remove it as well.

Update - Some more context, from what I can tell the last use of it was in def yarnpkg_version, removed here.

@stuhood

stuhood approved these changes Mar 7, 2019

Copy link
Member

left a comment

Thanks!

'Target {} should just use a sources argument. No BUILD files need changing. '
'The source argument will stop being populated -').format(target.type_alias),
)
if 'sources' in payload.as_dict():

This comment has been minimized.

Copy link
@codealchemy

codealchemy Mar 8, 2019

Author Contributor

I noticed this was last touched in 450de70 - it feels like we should be able to drop these checks / pops and allow an error to be raised if they're included (or move them to ignored_args?), @illicitonion @cosmicexplorer might ya'll know on if this can be removed, moved to ignored_args, or should be left as-is?

This comment has been minimized.

Copy link
@illicitonion

illicitonion Mar 8, 2019

Contributor

Yeah, I think you should be able to remove this entire block, and just treat sources like any other arg :) Good spot!

This comment has been minimized.

Copy link
@codealchemy

codealchemy Mar 11, 2019

Author Contributor

Getting rid of this caused quite a few test failures, most (maybe all? from what I've spot checked so far at least) of which are from having source= within BUILD files across pants. Would it be safe to assume those should be updated to sources=[...] accordingly, or might it be preferable to revert c09f9cb for now?

This comment has been minimized.

Copy link
@illicitonion

illicitonion Mar 11, 2019

Contributor

Oh dear - please revert that change and file a ticket assigned to me :) Sorry about that!

This comment has been minimized.

Copy link
@codealchemy

codealchemy Mar 11, 2019

Author Contributor

No worries! Will revert and get a ticket open for that.

Edit: #7357

This comment has been minimized.

Copy link
@Eric-Arellano

Eric-Arellano Mar 11, 2019

Contributor

Consider adding a TODO comment linking to 7357 here.

This comment has been minimized.

Copy link
@codealchemy

codealchemy Mar 11, 2019

Author Contributor

codealchemy added some commits Mar 7, 2019

Drop resolver-blacklist from python_setup
Removal version 1.13.0.dev2

@codealchemy codealchemy force-pushed the codealchemy:remove-dep-global-opt branch from b4703cc to c09f9cb Mar 8, 2019

@Eric-Arellano
Copy link
Contributor

left a comment

Thanks!

'Target {} should just use a sources argument. No BUILD files need changing. '
'The source argument will stop being populated -').format(target.type_alias),
)
if 'sources' in payload.as_dict():

This comment has been minimized.

Copy link
@Eric-Arellano

Eric-Arellano Mar 11, 2019

Contributor

Consider adding a TODO comment linking to 7357 here.

@stuhood

This comment has been minimized.

Copy link
Member

commented Mar 11, 2019

There were a few things that looked like network flakiness (failing to clone a git repo)... have restarted them. If they recur... then master might be broken as well.

@benjyw benjyw merged commit 5184b78 into pantsbuild:master Mar 13, 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.