Issue 29329056: Issue 3194 - Allow multiple sources for a dependency (Closed)
DescriptionIssue 3194 - Allow multiple sources for a dependency
Patch Set 1 #Patch Set 2 : Implemented Sebastian's suggested syntax #
Total comments: 6
Patch Set 3 : Addressed feedback #
Total comments: 4
Patch Set 4 : Addressed further feedback #
Total comments: 4
Patch Set 5 : Removed my username from the example #Patch Set 6 : Just return a list from merge_seqs instead of coercing the result into a tuple #MessagesTotal messages: 18
Patch Set 1
Well, this is simpler than I expected. Though, I'm still undecided whether we should add that feature. What are the actual use cases? The ticket didn't specify them. However, working around our currently broken git mirrors comes to mind. Anything else?
On 2015/10/12 15:57:35, Sebastian Noack wrote: > Well, this is simpler than I expected. Though, I'm still undecided whether we > should add that feature. What are the actual use cases? The ticket didn't > specify them. However, working around our currently broken git mirrors comes to > mind. Anything else? Yep, that's my main usecase so far. I wanted to specify an absolute Git URL for the adblockplus dependency of adblockpluschrome without it messing things up for Mercurial users. It seems an obvious limitation to me that you can't specify an absolute URL for a dependency when supporting multiple VCS systems. As you said the solution is pretty simple so I figured it was a worth doing, but it's up to you guys. I could also see the argument is that it's not worth adding any complexity here as it's a fairly rare thing to want to do.
I think, if we are going to support per-VCS absolute URLs, the syntax should rather be: adblockplus = hg:[<url>@]<rev> git:[<url>@]<rev> This feels more natural and consistent with the existing shorthand syntax.
My impression that absolute URLs in the dependencies file are an exception, a local override. Does it make sense to support multiple systems then?
On 2015/10/13 10:45:56, Wladimir Palant wrote: > My impression that absolute URLs in the dependencies file are an exception, a > local override. Does it make sense to support multiple systems then? As I said above, I'm also a little undecided. But to be fair, I can see following legit use cases: * Temporary workaround for broken mirrors or upstream repos. Not necessarily only local. * A fork, that in addition to upstream repos, also depends on some own repos. * Easier local tweaking during development. So I guess, I would accept a patch, with my feedback regarding the syntax addressed.
On 2015/10/13 11:06:40, Sebastian Noack wrote: > So I guess, I would accept a patch, with my feedback regarding the syntax > addressed. Done.
https://codereview.adblockplus.org/29329056/diff/29329152/ensure_dependencies.py File ensure_dependencies.py (right): https://codereview.adblockplus.org/29329056/diff/29329152/ensure_dependencies... ensure_dependencies.py:35: # Check out the adblockplus repository into adblockplus directory, overwriting Nit: "Check out" isn't hg/git terminology. https://codereview.adblockplus.org/29329056/diff/29329152/ensure_dependencies... ensure_dependencies.py:192: url_rev = re.match(source_regexp, value).groups() We decided a while ago to not use .match() anymore, but only .search(). That is because the behavior of .match(), implying end of string, but not not begin of string, is apparently quite confusing to some developers. https://codereview.adblockplus.org/29329056/diff/29329152/ensure_dependencies... ensure_dependencies.py:200: logging.warning("Ignoring invalid item '%s' for type %s" How about using %r instead '%s'?
Patch Set 3 : Addressed feedback https://codereview.adblockplus.org/29329056/diff/29329152/ensure_dependencies.py File ensure_dependencies.py (right): https://codereview.adblockplus.org/29329056/diff/29329152/ensure_dependencies... ensure_dependencies.py:35: # Check out the adblockplus repository into adblockplus directory, overwriting On 2015/10/15 15:19:41, Sebastian Noack wrote: > Nit: "Check out" isn't hg/git terminology. Done. https://codereview.adblockplus.org/29329056/diff/29329152/ensure_dependencies... ensure_dependencies.py:192: url_rev = re.match(source_regexp, value).groups() On 2015/10/15 15:19:41, Sebastian Noack wrote: > We decided a while ago to not use .match() anymore, but only .search(). That is > because the behavior of .match(), implying end of string, but not not begin of > string, is apparently quite confusing to some developers. Done. https://codereview.adblockplus.org/29329056/diff/29329152/ensure_dependencies... ensure_dependencies.py:200: logging.warning("Ignoring invalid item '%s' for type %s" On 2015/10/15 15:19:41, Sebastian Noack wrote: > How about using %r instead '%s'? (Some further testing showed it's clearer if all these values are quoted.) Done.
Sorry, found two more things. https://codereview.adblockplus.org/29329056/diff/29329157/ensure_dependencies.py File ensure_dependencies.py (right): https://codereview.adblockplus.org/29329056/diff/29329157/ensure_dependencies... ensure_dependencies.py:146: "^(?:(" + "|".join(repo_types.keys()) +"):)?" Please use map(re.escape, ...), just in case. https://codereview.adblockplus.org/29329056/diff/29329157/ensure_dependencies... ensure_dependencies.py:163: return tuple(i2 or i1 for i1, i2 in map(None, tuple_1 or (), tuple_2 or ())) While already using map, why not passing a function, but wrapping the result? def merge_tuples(seq1, seq2): return map(lambda item1, item2: item2 or item1, seq1 or (), seq2 or ()) Also I'm not happy about the function names. The logic isn'T specifc to tuples, neither does that name give any hint how these are merged. Not sure though whether there is a reasonable name to address the latter.
Patch Set 4 : Addressed further feedback https://codereview.adblockplus.org/29329056/diff/29329157/ensure_dependencies.py File ensure_dependencies.py (right): https://codereview.adblockplus.org/29329056/diff/29329157/ensure_dependencies... ensure_dependencies.py:146: "^(?:(" + "|".join(repo_types.keys()) +"):)?" On 2015/10/15 17:03:07, Sebastian Noack wrote: > Please use map(re.escape, ...), just in case. Done. https://codereview.adblockplus.org/29329056/diff/29329157/ensure_dependencies... ensure_dependencies.py:163: return tuple(i2 or i1 for i1, i2 in map(None, tuple_1 or (), tuple_2 or ())) On 2015/10/15 17:03:07, Sebastian Noack wrote: > While already using map, why not passing a function, but wrapping the result? > > def merge_tuples(seq1, seq2): > return map(lambda item1, item2: item2 or item1, seq1 or (), seq2 or ()) > > Also I'm not happy about the function names. The logic isn'T specifc to tuples, > neither does that name give any hint how these are merged. Not sure though > whether there is a reasonable name to address the latter. Good points and I agree that the function's name doesn't explain it's functionality too well. Only thing I could think to do was to add the documentation string, hopefully that will be enough to make this clear to the next person. Done.
https://codereview.adblockplus.org/29329056/diff/29329240/ensure_dependencies.py File ensure_dependencies.py (right): https://codereview.adblockplus.org/29329056/diff/29329240/ensure_dependencies... ensure_dependencies.py:163: return tuple(map(lambda item1, item2: item2 or item1, seq1 or (), seq2 or ())) Nit: Is coercing to a tuple really necessary? Won't list as returned by map do as well?
I didn't really review the code, but is the added complexity here still justified? https://codereview.adblockplus.org/29329056/diff/29329240/ensure_dependencies.py File ensure_dependencies.py (right): https://codereview.adblockplus.org/29329056/diff/29329240/ensure_dependencies... ensure_dependencies.py:40: adblockpluschrome = git:git@github.com:kzar/adblockpluschrome.git@1fad3a7 Nit: I don't think your username belongs in the examples here - it should be something generic like gituser.
On 2015/10/16 10:28:46, Wladimir Palant wrote: > I didn't really review the code, but is the added complexity here still > justified? Frankly, I don't have a strong opinion, whether the added complexity is worth it. But if we allow absolute per-VCS URLs, I prefer this approach over the the hack initially suggested. But if you have any ideas how to simplify it, I'm happy to hear them.
Patch Set 5 : Removed my username from the example Patch Set 6 : Just return a list from merge_seqs instead of coercing the result into a tuple https://codereview.adblockplus.org/29329056/diff/29329240/ensure_dependencies.py File ensure_dependencies.py (right): https://codereview.adblockplus.org/29329056/diff/29329240/ensure_dependencies... ensure_dependencies.py:40: adblockpluschrome = git:git@github.com:kzar/adblockpluschrome.git@1fad3a7 On 2015/10/16 10:28:45, Wladimir Palant wrote: > Nit: I don't think your username belongs in the examples here - it should be > something generic like gituser. Done. https://codereview.adblockplus.org/29329056/diff/29329240/ensure_dependencies... ensure_dependencies.py:163: return tuple(map(lambda item1, item2: item2 or item1, seq1 or (), seq2 or ())) On 2015/10/16 10:24:49, Sebastian Noack wrote: > Nit: Is coercing to a tuple really necessary? Won't list as returned by map do > as well? You're right, a list will do. (I figured a tuple would be better as we're not planning to mutate it, but on the other hand it's kind of pointless to coerce a list that's already suitable for our needs.) Done.
Looks good to me now. @Wladimir: How strong do you feel strong about the added complexity? @kzar: What do you think?
Well I'm pretty happy with the code now, I think if anything the changes make things easier to understand. I've tried very hard to make it as simple as possible and well commented. I also like the new syntax it supports. My changes adds 30 lines to the overall count, I think 25 of those are either whitespace or comments. However if you guys feel that the complexity isn't worth it I would understand too. I realise the new syntax would so far be rarely required, even though I personally would find it useful.
LGTM form my side. 
|
