Issue 4542760560361472: Issue 2310 - Handle alternative syntax of Git SSH URLs (Closed)
DescriptionIssue 2310 - Handle alternative syntax of Git SSH URLs
Patch Set 1 #
Total comments: 1
Patch Set 2 : Moved logic into postprocessing function #
Total comments: 2
Patch Set 3 : Moved postprocess_url function into VC abstraction classes. #
Total comments: 8
Patch Set 4 : Avoid looking up postprocess_url function twice. #MessagesTotal messages: 12
Patch Set 1
http://codereview.adblockplus.org/4542760560361472/diff/5629499534213120/ensu... File ensure_dependencies.py (right): http://codereview.adblockplus.org/4542760560361472/diff/5629499534213120/ensu... ensure_dependencies.py:197: else: This seems to be the wrong place to do it. What we really need to do is post-process Git "URLs" - all of them. So I'd expect something like: root = postprocess_url(roots[type], type) sourcename = postprocess_url(sourcename, type) And the logic joining URLs should be unchanged, merely with roots[type] replaced by root.
Patch Set 2 : Moved logic into postprocessing function
http://codereview.adblockplus.org/4542760560361472/diff/5724160613416960/ensu... File ensure_dependencies.py (right): http://codereview.adblockplus.org/4542760560361472/diff/5724160613416960/ensu... ensure_dependencies.py:180: if (type == "git" and "@" in url and ":" in url and not urlparse.urlsplit(url).scheme): No explicit VCS switches please. We already have an abstraction layer in place, where stuff like this belongs.
Patch Set 3 : Moved postprocess_url function into VC abstraction classes. http://codereview.adblockplus.org/4542760560361472/diff/5724160613416960/ensu... File ensure_dependencies.py (right): http://codereview.adblockplus.org/4542760560361472/diff/5724160613416960/ensu... ensure_dependencies.py:180: if (type == "git" and "@" in url and ":" in url and not urlparse.urlsplit(url).scheme): On 2015/04/30 13:08:28, Sebastian Noack wrote: > No explicit VCS switches please. We already have an abstraction layer in place, > where stuff like this belongs. Done.
http://codereview.adblockplus.org/4542760560361472/diff/5676830073815040/ensu... File ensure_dependencies.py (right): http://codereview.adblockplus.org/4542760560361472/diff/5676830073815040/ensu... ensure_dependencies.py:113: return "ssh://" + url.replace(":", "/", 1) How does git URLs specify ports? http://codereview.adblockplus.org/4542760560361472/diff/5676830073815040/ensu... ensure_dependencies.py:199: root = repo_types[type].postprocess_url(roots[type]) Nit, don't repeat yourself: postprocess_url = repo_types[type].postprocess_url
Patch Set 4 : Avoid looking up postprocess_url function twice. http://codereview.adblockplus.org/4542760560361472/diff/5676830073815040/ensu... File ensure_dependencies.py (right): http://codereview.adblockplus.org/4542760560361472/diff/5676830073815040/ensu... ensure_dependencies.py:113: return "ssh://" + url.replace(":", "/", 1) On 2015/04/30 13:34:36, Sebastian Noack wrote: > How does git URLs specify ports? Apparently they can't, you have to specify non-standard ports in your `~/.ssh/config`. http://stackoverflow.com/questions/1558719/using-a-remote-repository-with-non... http://codereview.adblockplus.org/4542760560361472/diff/5676830073815040/ensu... ensure_dependencies.py:199: root = repo_types[type].postprocess_url(roots[type]) On 2015/04/30 13:34:36, Sebastian Noack wrote: > Nit, don't repeat yourself: > > postprocess_url = repo_types[type].postprocess_url Done.
LGTM
LGTM http://codereview.adblockplus.org/4542760560361472/diff/5676830073815040/ensu... File ensure_dependencies.py (right): http://codereview.adblockplus.org/4542760560361472/diff/5676830073815040/ensu... ensure_dependencies.py:199: root = repo_types[type].postprocess_url(roots[type]) On 2015/04/30 13:34:36, Sebastian Noack wrote: > Nit, don't repeat yourself: > > postprocess_url = repo_types[type].postprocess_url Rather questionable advise in this context IMHO.
http://codereview.adblockplus.org/4542760560361472/diff/5676830073815040/ensu... File ensure_dependencies.py (right): http://codereview.adblockplus.org/4542760560361472/diff/5676830073815040/ensu... ensure_dependencies.py:199: root = repo_types[type].postprocess_url(roots[type]) On 2015/04/30 15:48:30, Wladimir Palant wrote: > On 2015/04/30 13:34:36, Sebastian Noack wrote: > > Nit, don't repeat yourself: > > > > postprocess_url = repo_types[type].postprocess_url > > Rather questionable advise in this context IMHO. How is caching one variable lookup + one item lookup + another attribute lookup into a variable, reducing duplication, questionable?
http://codereview.adblockplus.org/4542760560361472/diff/5676830073815040/ensu... File ensure_dependencies.py (right): http://codereview.adblockplus.org/4542760560361472/diff/5676830073815040/ensu... ensure_dependencies.py:199: root = repo_types[type].postprocess_url(roots[type]) On 2015/04/30 17:58:28, Sebastian Noack wrote: > How is caching one variable lookup + one item lookup + another attribute lookup > into a variable, reducing duplication, questionable? If it provides a negligible performance gain while reducing readability and consistency with the surrounding code - sure, that's questionable. It's called "premature optimization."
http://codereview.adblockplus.org/4542760560361472/diff/5676830073815040/ensu... File ensure_dependencies.py (right): http://codereview.adblockplus.org/4542760560361472/diff/5676830073815040/ensu... ensure_dependencies.py:199: root = repo_types[type].postprocess_url(roots[type]) On 2015/04/30 18:34:07, Wladimir Palant wrote: > On 2015/04/30 17:58:28, Sebastian Noack wrote: > > How is caching one variable lookup + one item lookup + another attribute > lookup > > into a variable, reducing duplication, questionable? > > If it provides a negligible performance gain while reducing readability and > consistency with the surrounding code - sure, that's questionable. It's called > "premature optimization." I didn't even mention performance. "Don't repeat yourself" is mostly about code readability ans maintainability. And I don't see how duplicating code helps there. 
|
