[docs] Update the cache section on the Task developer page. #4152

Merged
merged 3 commits into from Dec 30, 2016

Conversation

Projects
None yet
2 participants
@mateor
Member

mateor commented Dec 28, 2016

Problem

The existing example had a bug, in that it was only checking the cache
for a VTS of the invalid_vts. It also was a little long and hand waved over
the code snippets in places.

Solution

I fixed the examples and simplified them some.

The manual caching section may be marginally non-optimal, in that it
doesn't update the cache with all pairs at once, but once per vt.
But we do this within jvm_compile, so it isn't like it doesn't happen.
And that allowed to remove the unimplemented match_up method call.

This also separates the VersionedTargetSets out of the manual caching
section since using VTS does not necessarily mean that the task dev must
upload the VTS manually.

Result

I believe the examples have improved correctness/clarity the section
is about 25 lines shorter as a bonus.

[docs] Update the cache section on the Task developer page.
The existing example had a bug, in that it was only checking the cache
for a VTS of the invalid_vts.

I also simplified the example to only the pertinent parts. It is possibly
marginally not optimal, in that it doesn't update the cache with all
pairs at once, but once per vt. But we do this within jvm_compile,
so it isn't like it doesn't happen. And I think that this is much
clearer about what needs to happen, since it doesn't offload the
pair construction to an unimplemented match_up function.

I believe the examples are a bit clearer and the section is about
25 lines shorter as a bonus.
@mateor

This comment has been minimized.

Show comment
Hide comment
@mateor

mateor Dec 28, 2016

Member

Here is what publishing the docs locally looks like:

screen shot 2016-12-28 at 4 28 57 pm

Member

mateor commented Dec 28, 2016

Here is what publishing the docs locally looks like:

screen shot 2016-12-28 at 4 28 57 pm

@mateor mateor requested review from kwlzn, benjyw and wisechengyi Dec 28, 2016

@stuhood

Thanks.

src/docs/dev_tasks.md
-this is Ivy resolution, where the set of resolved 3rd party dependencies is a property
-of all targets taken together, not of each target individually.
+ def execute(self):
+ targets = self.context.targets()

This comment has been minimized.

@stuhood

stuhood Dec 28, 2016

Member

Would be good to add some imaginary predicate arg here, to demonstrate that you generally wouldn't do this for all targets in the context.

@stuhood

stuhood Dec 28, 2016

Member

Would be good to add some imaginary predicate arg here, to demonstrate that you generally wouldn't do this for all targets in the context.

This comment has been minimized.

@mateor

mateor Dec 29, 2016

Member

Yeah, I almost did this myself. Will add.

@mateor

mateor Dec 29, 2016

Member

Yeah, I almost did this myself. Will add.

mateor added some commits Dec 29, 2016

Add a predicate the the example target gathering
By request, and I agree. Only core tasks should not filter by
Target type. Anyone reading these docs should filter, this makes
it clear.

I prefer to use the predicate=fn personally, but my goal with the
sample code was for it to be complete and run if pasted in a
task. Even something like self._is_your_target is hand wavy if
you are a first time task developer.
@mateor

This comment has been minimized.

Show comment
Hide comment
@mateor

mateor Dec 29, 2016

Member

Here is how it looks with the predicate added, as well as adding VT description.

screen shot 2016-12-29 at 4 20 30 pm

Member

mateor commented Dec 29, 2016

Here is how it looks with the predicate added, as well as adding VT description.

screen shot 2016-12-29 at 4 20 30 pm

@mateor mateor merged commit 9d39bef into pantsbuild:master Dec 30, 2016

1 check passed

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

lenucksi added a commit to lenucksi/pants that referenced this pull request Apr 25, 2017

[docs] Update the cache section on the Task developer page. (#4152)
The existing example had a bug, in that it was only checking the cache
for a VTS of the invalid_vts. I also simplified the examples.

The new example is possibly marginally not optimal, in that it doesn't update
the cache with all pairs at once, but once per vt. But we do this within jvm_compile,
so it isn't like it doesn't happen. But I think that this is much
clearer about what needs to happen, since it doesn't offload the
pair construction to an unimplemented match_up function.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment