#general
- b
bored-art-40741
08/10/2015, 7:26 PMThe API is the same, but the semantics are different, which is specifically why I'm concerned about this change - b
bored-art-40741
08/10/2015, 7:26 PMThis is a bugfix for changed goals, but it could be a regression for goals that do not want to see a target claim ownership for a file that doesn't exist just because it would have owned it if it did exist - b
bored-art-40741
08/10/2015, 7:27 PMOh, okay. So this both breaks buildgen, and I think is a performance regression as well - g
gorgeous-fish-11040
08/10/2015, 7:28 PMBut such targets should be claimed as changed! Maybe users of the mapper were expecting it but it doesn’t work for them. - b
bored-art-40741
08/10/2015, 7:29 PM
isn't the only consumer of thischanged - b
bored-art-40741
08/10/2015, 7:29 PMThat's my point - b
bored-art-40741
08/10/2015, 7:29 PMThis is a utility for mapping files actually owned by targets to the targets that own them - b
bored-art-40741
08/10/2015, 7:29 PMIn a performant way - b
bored-art-40741
08/10/2015, 7:30 PMChanged used it, but as you've noted changed has a slightly different need in that it would also like to know if a target would own a file if it existed - b
bored-art-40741
08/10/2015, 7:31 PMIn other words:
should be left alone (for now) and a newLazySourceMapper
should be written forSourceMapper
to usechanged - b
bored-art-40741
08/10/2015, 7:31 PMPerhaps an interface should be defined that both of them implement - b
bored-art-40741
08/10/2015, 7:32 PMYes, that will mean that there are no OSS pants consumers of
, but we still consume it internally.LazySourceMapper - g
gorgeous-fish-11040
08/10/2015, 7:36 PMI was going to do performance testing as well to verify there are no regressions. Regarding keeping the old implementation I’m a bit concerned. Eventually it should be removed if there are no usages in Pants OSS. So let’s keep it deprecated for now so you’ll be able to move to the new one or move
to Foursquare internal. Does it sound good?LazySourceMapper - b
bored-art-40741
08/10/2015, 7:37 PMWhen buildgen is open sourced it will be used again, so I don't anticipate it actually getting removed - p
proud-alligator-68508
08/10/2015, 7:38 PMsorry, catching up. How, if at all, does that affect
/changed
performance?compile-changed - b
bored-art-40741
08/10/2015, 7:38 PMBut yes, in the absence of usage in OSS pants I would probably expect it to be deprecated and removed eventually - b
bored-art-40741
08/10/2015, 7:38 PMThough it's worth noting that providing utilities for other plugin authors to use is totally legitimate - b
bored-art-40741
08/10/2015, 7:38 PMEven if pants doesn't use them internally - g
gorgeous-fish-11040
08/10/2015, 7:39 PM@bored-art-40741: BTW I can add a check for files to be presented on disk so you can configure it - p
proud-alligator-68508
08/10/2015, 7:39 PMyeah,
is more supposed to be a scheduling engine plus utilities you can stick together in one or more plugins right?pants.base - b
bored-art-40741
08/10/2015, 7:39 PMSuch a check combined with the way you refactored it would almost certainly be a perf regression - g
gorgeous-fish-11040
08/10/2015, 7:41 PMfor some cases. but I think walking a file tree is also pretty expensive especially for rglobs cases. that’s why I want to do some perf testing as well - b
bored-art-40741
08/10/2015, 7:41 PMThose some cases are relevant for us. I think the best thing to do here is to split the implementations - b
bored-art-40741
08/10/2015, 7:42 PMThey have different semantics and as a result end up having different implementations to maintain performance - g
gorgeous-fish-11040
08/10/2015, 7:44 PMmake sense. will keep both of them with a common base class. - b
bored-art-40741
08/10/2015, 7:45 PMsgtm - b
bored-art-40741
08/10/2015, 7:45 PMThanks - w
witty-crayon-22786
08/10/2015, 9:41 PM@enough-analyst-54434: i asked @adventurous-twilight-44110 to split his "local sources" and "remote libraries" golang reviews... do you think we're ready to get the first one in? - w
witty-crayon-22786
08/10/2015, 9:42 PMit's already generally useful if someone has vendored-in all of their deps as source. - w
witty-crayon-22786
08/10/2015, 9:43 PM@happy-kitchen-89482: i was planning to add some more information about options registration to the Task page, but it looks like you might be working on a top level page perhaps?