Code review comment for lp://qastaging/~danilo/linaro-android-build-tools/no-seed

Revision history for this message
Данило Шеган (danilo) wrote :

That would still lead to code that has to check for several conditions: eg. it'd become something like

  remote_url = remote.get('fetch')
  # Try to see if there's a remote URL
  target_url = rewrite_urls.get(remote_url,
                                rewrite_urls.get(remote_url.rstrip('/'), None))
  if target_url is not None:
     remote.set('fetch', target_url)

(Or, I could repeat the if a few times, but I'd hate that even more).

I don't think the above would be clear enough, and would be easy to break (one would have to ensure URLs in rewrite_urls are without the '/': either with more code, or by trying not to forget that :), and all to save a few measly almost-duplicated lines which would lead to:

  remote_url = remote.get('fetch')
  if remote_url in rewrite_urls:
     remote.set('fetch', rewrite_urls[remote_url])

On top of all this, I don't want to worry about whether repo cares or doesn't care about '/', and I want to copy whatever is being done in the manifest (if there's a trailing '/', I want rewritten URL to have it as well). Sure, we can add more code for that, but it'd make it even less clear what's going on, and it would be longer.

At this time, I want to live with this duplication, but if you really insist, I'll add the code to deal with the slash.

« Back to merge proposal