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

Put the ChangeCalculator implementation next to TargetRootsCalculator #5917

Merged
merged 5 commits into from Jun 8, 2018

Conversation

Projects
None yet
3 participants
@alanbato
Copy link
Member

alanbato commented Jun 6, 2018

Problem

While working on #5912, I found the code related to target calculation (currently just used along the --changed option and thus ChangeCalculator very disperse.

Solution

After discussing it with @stuhood, we came to the conclusion that bringing the code together and simplifying the dependencies would make this part of the codebase easier to work with. And thus this PR aims to do just that.

Result

The user should not be aware of this change. It is internal codebase refactoring.

PS. I skipped the pre-commit hook due to a bug with my isort.sh script, so it may fail on CI before I fix the import order.

@stuhood stuhood requested review from stuhood and kwlzn Jun 6, 2018

@stuhood
Copy link
Member

stuhood left a comment

Thanks!



class EngineChangeCalculator(object):
"""A ChangeCalculator variant that uses the v2 engine for source mapping."""

This comment has been minimized.

@stuhood

stuhood Jun 6, 2018

Member

This comment and the classname are probably both stale: can just take the name ChangeCalculator at this point.

This comment has been minimized.

@kwlzn

from __future__ import (absolute_import, division, generators, nested_scopes, print_function,
unicode_literals, with_statement)

This comment was marked as resolved.

@stuhood

stuhood Jun 6, 2018

Member

Since this file is gone, you should delete the target that refers to it from src/python/pants/scm/BUILD... afterward, you should run ./pants list :: to confirm that all references to src/python/pants/scm:change_calculator are deleted.




class EngineChangeCalculator(object):

This comment has been minimized.

@kwlzn

kwlzn Jun 6, 2018

Member

I'm not crazy about merging these two classes into the same file - it doesn't feel logically connected to me and feels like it's muddying the water a bit with the combined imports etc.

so at a minimum, I think EngineChangeCalculator and friends should go into its own separate file.

but I'm not fully convinced that pants.init.* would be any better of an namespace for it other than just being nearer by namespace to TargetRootsCalculator? the reason *ChangeCalculator lives in pants.scm.* is because it uses an SCM under the hood (git) to detect changes and is backed by other code in pants.scm.*. imho, it would be just as easy to import and use that here than to move it.

wdyt?

This comment has been minimized.

@stuhood

stuhood Jun 6, 2018

Member

I suggested this as one possible refactoring because @alanbato is about to add an OwnersCalculator as a neighbor to ChangeCalculator, which implements the third strategy for "target root calculation".

but I'm not fully convinced that pants.init.* would be any better of an namespace for it other than just being nearer by namespace to TargetRootsCalculator? the reason ChangeCalculator lives in pants.scm. is because it uses an SCM under the hood (git) to detect changes and is backed by other code in pants.scm.*. imho, it would be just as easy to import and use that here than to move it.

ChangeCalculator has very little scm specific logic, and instead has almost entirely "target root selection" logic.

You could almost think of ChangeCalculator, OwnersCalculator and LiteralCalculator as the three TargetRootsCalculator strategies... having them live in separate places just obscures that, imo.

This comment has been minimized.

@kwlzn

kwlzn Jun 6, 2018

Member

gotcha. grouping them on that basis sounds good to me, but I'm still leaning toward separate files per Calculator impl to keep things tidy (~"one class per file")?

would sticking them in a nested package (e.g. src/python/pants/init/roots/*_calculator.py et al -> e.g. from pants.init.roots.change_calculator import ChangeCalculator) suffice as the grouping? or is that too granular?

This comment has been minimized.

@stuhood

stuhood Jun 7, 2018

Member

(~"one class per file")?

Hm... it already wasn't "one class per file" in that _DependentGraph is in the same file, and used by both implementations. Not sure I agree about splitting them out unless we have a better strategy for that.

This comment has been minimized.

@kwlzn

kwlzn Jun 7, 2018

Member

that's where the "~" comes in ;)

but sure, whatever you think.

@kwlzn

kwlzn approved these changes Jun 6, 2018

Copy link
Member

kwlzn left a comment

(otherwise lgtm!)



class EngineChangeCalculator(object):
"""A ChangeCalculator variant that uses the v2 engine for source mapping."""

This comment has been minimized.

@kwlzn

@alanbato alanbato force-pushed the alanbato:centralize_target_calculation branch from bc6dfee to cc66be6 Jun 6, 2018

@alanbato

This comment has been minimized.

Copy link
Member

alanbato commented Jun 6, 2018

Ping, made the requested changes :) Travis will probably still fail due to me not being able to isort the project. If you can help me with that @stuhood that'd be awesome :)

@alanbato alanbato force-pushed the alanbato:centralize_target_calculation branch from a93665d to 506360a Jun 6, 2018

@alanbato

This comment has been minimized.

Copy link
Member

alanbato commented Jun 6, 2018

@stuhood: If you could review the changes and the tests I fixed that'd be awesome :)

@@ -27,7 +27,7 @@
from pants.engine.rules import SingletonRule
from pants.engine.scheduler import Scheduler
from pants.option.global_options import GlobMatchErrorBehavior
from pants.scm.change_calculator import EngineChangeCalculator
from pants.option.options_bootstrapper import OptionsBootstrapper

This comment has been minimized.

@stuhood

stuhood Jun 7, 2018

Member

This import looks unused. I believe that if isort weren't failing, this would show up as the next lint failure.

This comment has been minimized.

@alanbato

alanbato Jun 7, 2018

Member

It looks like it's being used in L123 though

This comment has been minimized.

@stuhood

stuhood Jun 7, 2018

Member

Unless a symbol is literally referenced (spelled out) it doesn't need to be imported. But the CI can confirm.




class EngineChangeCalculator(object):

This comment has been minimized.

@stuhood

stuhood Jun 7, 2018

Member

(~"one class per file")?

Hm... it already wasn't "one class per file" in that _DependentGraph is in the same file, and used by both implementations. Not sure I agree about splitting them out unless we have a better strategy for that.

@stuhood

stuhood approved these changes Jun 7, 2018

Copy link
Member

stuhood left a comment

Looks good to me, although we need to get isort fixed for you so that you can see the remaining lint errors.

To unblock the PR, invoking the downloaded isort.pex directly on all files should work to expose more lint errors in CI. Can merge tomorrow as soon as CI is green.



class ChangeCalculator(object):
"""A ChangeCalculator that find the targed addresses of changed files based on scm."""

This comment has been minimized.

@stuhood

stuhood Jun 7, 2018

Member

s/targed/target/

@alanbato alanbato force-pushed the alanbato:centralize_target_calculation branch from 4b86aa4 to 90fd9d4 Jun 7, 2018

@alanbato

This comment has been minimized.

Copy link
Member

alanbato commented Jun 7, 2018

So I did like you suggested and ran the isort.pex on src/ and sorted the import order. also the target part. I think the CI is going to pass now :)
After this is merged I'll continue the work on the OwnerCalculator

@stuhood

This comment has been minimized.

Copy link
Member

stuhood commented Jun 7, 2018

@alanbato : Yea, some fixes are needed: https://travis-ci.org/pantsbuild/pants/jobs/389339946#L927

Let's keep iterating on getting isort fixed for you. But in the meantime, you should be able to run ./pants -q --changed-parent=master lint to check for these kind of errors.

@stuhood stuhood merged commit 7f27a74 into pantsbuild:master Jun 8, 2018

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