Issue 4653 - Add handling for bookmarks to readMetadata

Issue 4653 - Add handling for bookmarks to readMetadata

Repository: hg.adblockplus.org/sitescripts

[Sebastian Noack, 2016-11-25 13:01:56]

https://codereview.adblockplus.org/29364443/diff/29364529/sitescripts/extensi...
File sitescripts/extensions/utils.py (right):

https://codereview.adblockplus.org/29364443/diff/29364529/sitescripts/extensi...
sitescripts/extensions/utils.py:210: metadata = self.readMetadata(self.revision)
I think this logic belongs into readMetadata(), so that when no arguments are
supplied that it uses self.revision by default. Now, where we have that value,
it seems more expected that it is used by default rather than tip.

https://codereview.adblockplus.org/29364443/diff/29364529/sitescripts/extensi...
sitescripts/extensions/utils.py:320: message = 'No download link found for repo:
' + repo
Any reason you don't pass the message directly to the Exception() constructor,
but assign it to a variable first?

[Jon Sonesen, 2016-11-25 13:05:01]

https://codereview.adblockplus.org/29364443/diff/29364529/sitescripts/extensi...
File sitescripts/extensions/utils.py (right):

https://codereview.adblockplus.org/29364443/diff/29364529/sitescripts/extensi...
sitescripts/extensions/utils.py:210: metadata = self.readMetadata(self.revision)
On 2016/11/25 13:01:56, Sebastian Noack wrote:
> I think this logic belongs into readMetadata(), so that when no arguments are
> supplied that it uses self.revision by default. Now, where we have that value,
> it seems more expected that it is used by default rather than tip.

I agree here.

https://codereview.adblockplus.org/29364443/diff/29364529/sitescripts/extensi...
sitescripts/extensions/utils.py:320: message = 'No download link found for repo:
' + repo
On 2016/11/25 13:01:56, Sebastian Noack wrote:
> Any reason you don't pass the message directly to the Exception() constructor,
> but assign it to a variable first?

I had it in an if statement when i was testing it and forgot to change it, will
pass the message directly

[Sebastian Noack, 2016-11-25 13:40:29]

LGTM

https://codereview.adblockplus.org/29364443/diff/29364529/sitescripts/extensi...
File sitescripts/extensions/utils.py (right):

https://codereview.adblockplus.org/29364443/diff/29364529/sitescripts/extensi...
sitescripts/extensions/utils.py:210: metadata = self.readMetadata(self.revision)
On 2016/11/25 13:05:01, Jon Sonesen wrote:
> On 2016/11/25 13:01:56, Sebastian Noack wrote:
> > I think this logic belongs into readMetadata(), so that when no arguments
are
> > supplied that it uses self.revision by default. Now, where we have that
value,
> > it seems more expected that it is used by default rather than tip.
> 
> I agree here.

For reference, we discussed that a little more on IRC, and as it turned out that
there is no place left where readMetadata() is called without argument, we
decided to leave the code here as is, but make the argument mandatory.

[Vasily Kuznetsov, 2016-11-25 13:47:24]

LGTM