Issue 29339623: Issue 3681 - Add suport for "Fixes XXXX - ..." commit messages (Closed)
|
Created:
April 8, 2016, 7:32 p.m. by Vasily Kuznetsov Modified:
May 18, 2016, 8:39 a.m. CC:
mathias Visibility:
Public. |
DescriptionIssue 3681 - Add suport for "Fixes XXXX - ..." commit messages
Patch Set 1 #
Total comments: 12
Patch Set 2 : Split into two hooks to handle the master bookmark, move documentation to README, add tests #
Total comments: 26
Patch Set 3 : Address review comments #
Total comments: 9
Patch Set 4 : Address comments on patch set 3 #
Total comments: 4
Patch Set 5 : Fix the warnings flagged by flake8-abp and the capitalisation of Platform in the config example #
Total comments: 17
Patch Set 6 : Fix Strunk+White violations #
MessagesTotal messages: 27
Hi Felix, I thought you would like to review this since you were the author of the original hook. The patchset is not final at this moment -- I would like to extend your test suite to test the new functionality once that commit lands in the repository. Anyway, I thought we could already start reviewing the hook itself -- as you can see there's a lot of new code, in particular the milestone handling logic is not completely trivial. I also chose to extend the status reporting when updating issues, so now it says what exactly it did to the issue. That wasn't part of the ticket, but I think this functionality would be useful, at least when we start using this, to avoid surprises if, for example, an unexpected milestone gets chosen and it's only noticed much later. Also if you don't have time to review, as might very well be the case, feel free to delegate. Have a nice weekend, Vasily
It seems that this didn't get sent out on Friday for some reason.
On 2016/04/10 14:31:35, Vasily Kuznetsov wrote: > It seems that this didn't get sent out on Friday for some reason. Oh it did, I just didn't get around to looking at this yet :) I'll gladly review it (but I probably won't manage before the end of the week), but module owner or peer should always be reviewers. So can you add Sebastian or Dave as reviewers here? I would add Dave I guess, since he suggested this, and Sebastian is already in CC, he can jump in if he wants to.
> I'll gladly review it... Yes please Felix, you're much more failure with this code than me. That said I've made a quick start, more to follow... Also I guess with the discussion around your other review maybe we should use 4 space indent here and otherwise follow PEP8 even where it makes things inconsistent with the rest of the sitescripts code? https://codereview.adblockplus.org/29339623/diff/29339624/sitescripts/hg/bin/... File sitescripts/hg/bin/update_issues.py (right): https://codereview.adblockplus.org/29339623/diff/29339624/sitescripts/hg/bin/... sitescripts/hg/bin/update_issues.py:16: """Hook for integrating Mercurial with Trac. Mind putting this documentation in a README (sitescripts/hg/README.md) instead? That's what we've been doing for some of the other parts of sitescripts, for example content_blocker_lists. https://codereview.adblockplus.org/29339623/diff/29339624/sitescripts/hg/bin/... sitescripts/hg/bin/update_issues.py:33: assigned to it if configured and unless the issue already has a milestone. We should also avoid assigning milestones and closing issues if the commit was pushed to any branch other than master. Mind adding a note about that here? https://codereview.adblockplus.org/29339623/diff/29339624/sitescripts/hg/bin/... sitescripts/hg/bin/update_issues.py:57: class _Update(object): I guess I don't understand the need for this class, we could just use a dictionary right? https://codereview.adblockplus.org/29339623/diff/29339624/sitescripts/hg/bin/... sitescripts/hg/bin/update_issues.py:109: yield module, re.compile("^" + regexp + "$", re.I) I don't think this regexp should be case-insensitive. https://codereview.adblockplus.org/29339623/diff/29339624/sitescripts/hg/bin/... sitescripts/hg/bin/update_issues.py:183: updates = _create_issue_updates(ui, repo, node) Couldn't we instead iterate through the updates once, doing the various processing tasks as we go? (A loop in one place, calling various processing functions for each update.) It seems kind of weird to me how we're creating the `updates` generator(?) over and over, and iterating through it each time in the various processing functions.
On 2016/04/18 14:48:30, kzar wrote: > > I'll gladly review it... > Yes please Felix, you're much more failure with this code than me. Oh god, of course that should have said "...much more familiar..."!
Finally, the updated version, where we introduce a second hook to enable the logic related to `master` bookmark, move the documentation to README, add unit tests and generally rewrite and reformat the code. https://codereview.adblockplus.org/29339623/diff/29339624/sitescripts/hg/bin/... File sitescripts/hg/bin/update_issues.py (right): https://codereview.adblockplus.org/29339623/diff/29339624/sitescripts/hg/bin/... sitescripts/hg/bin/update_issues.py:16: """Hook for integrating Mercurial with Trac. On 2016/04/18 14:48:30, kzar wrote: > Mind putting this documentation in a README (sitescripts/hg/README.md) instead? > That's what we've been doing for some of the other parts of sitescripts, for > example content_blocker_lists. Done. https://codereview.adblockplus.org/29339623/diff/29339624/sitescripts/hg/bin/... sitescripts/hg/bin/update_issues.py:33: assigned to it if configured and unless the issue already has a milestone. On 2016/04/18 14:48:30, kzar wrote: > We should also avoid assigning milestones and closing issues if the commit was > pushed to any branch other than master. Mind adding a note about that here? Oh damn! I missed this in the original code. Now the issue is updated to make this more clear and the code is separated into two hooks (it seems that's the only way to track the movement of the `master` bookmark properly). https://codereview.adblockplus.org/29339623/diff/29339624/sitescripts/hg/bin/... sitescripts/hg/bin/update_issues.py:57: class _Update(object): On 2016/04/18 14:48:30, kzar wrote: > I guess I don't understand the need for this class, we could just use a > dictionary right? Tried to make the code below prettier/more readable. But you're right, it's not worth it, and `_Update` is redundant. I will remove it. Done https://codereview.adblockplus.org/29339623/diff/29339624/sitescripts/hg/bin/... sitescripts/hg/bin/update_issues.py:109: yield module, re.compile("^" + regexp + "$", re.I) On 2016/04/18 14:48:30, kzar wrote: > I don't think this regexp should be case-insensitive. My reasoning was that we want to be more forgiving to people who configure the hook and create milestones, so that if they mess up the case, the hook won't misbehave. The errors could easily go undetected, so we better try to make it work in the hook. Another reason is that the regexps for the commit message are case insensitive (even though the errors there are more likely to be seen), so I thought that would be consistent with this approach of being not completely strict. Do you think there's a chance that the case-insensitive regexp will match something it shouldn't match? If not, what arguments do you see for regexp being case-sensitive? https://codereview.adblockplus.org/29339623/diff/29339624/sitescripts/hg/bin/... sitescripts/hg/bin/update_issues.py:183: updates = _create_issue_updates(ui, repo, node) On 2016/04/18 14:48:30, kzar wrote: > Couldn't we instead iterate through the updates once, doing the various > processing tasks as we go? (A loop in one place, calling various processing > functions for each update.) It seems kind of weird to me how we're creating the > `updates` generator(?) over and over, and iterating through it each time in the > various processing functions. We're basically creating a stream of updates and send it through a bunch of handlers that filter/change the updates. I agree, the naming looks a bit weird in these `updates = foo(updates)`, `updates = bar(updates)` lines, but the code is simpler this way. After separating the logic into two hooks (that was necessary to react to the movements of `master` bookmark) the logic of each of the hooks became simple enough that it was possible to change it from this streaming pattern to more imperative style. So, Done.
Bump.
On 2016/04/29 12:10:16, Vasily Kuznetsov wrote: > Bump. Just to be clear: I'm waiting for kzar's approval before I have a look. If you were waiting for me kzar, I can go first.
On 2016/04/29 12:14:12, Felix Dahlke wrote: > On 2016/04/29 12:10:16, Vasily Kuznetsov wrote: > > Bump. > > Just to be clear: I'm waiting for kzar's approval before I have a look. If you > were waiting for me kzar, I can go first. Sorry, not waiting for you just being a bit slow! Once what I'm working on is under review I'll come back to this, hopefully early next week.
On 2016/04/29 12:15:56, kzar wrote: > On 2016/04/29 12:14:12, Felix Dahlke wrote: > > On 2016/04/29 12:10:16, Vasily Kuznetsov wrote: > > > Bump. > > > > Just to be clear: I'm waiting for kzar's approval before I have a look. If you > > were waiting for me kzar, I can go first. > > Sorry, not waiting for you just being a bit slow! Once what I'm working on is > under review I'll come back to this, hopefully early next week. Thanks for the updates. Meanwhile, there's this thing with `sitescripts.templateFilters.formatBugLinks` being affected by this change. I haven't touched it so far. Can anyone point me to where this function is used? Otherwise I'm lost about how I would test any changes in it.
> Meanwhile, there's this thing with being affected by this change. I haven't touched it so far. Can anyone point me to where this function [sitescripts.templateFilters.formatBugLinks] is used? I'm not sure, I would try grepping for it through our repositories or asking Wladimir. https://codereview.adblockplus.org/29339623/diff/29339624/sitescripts/hg/bin/... File sitescripts/hg/bin/update_issues.py (right): https://codereview.adblockplus.org/29339623/diff/29339624/sitescripts/hg/bin/... sitescripts/hg/bin/update_issues.py:109: yield module, re.compile("^" + regexp + "$", re.I) On 2016/04/24 21:49:50, Vasily Kuznetsov wrote: > On 2016/04/18 14:48:30, kzar wrote: > > I don't think this regexp should be case-insensitive. > > My reasoning was that we want to be more forgiving to people who configure the > hook and create milestones, so that if they mess up the case, the hook won't > misbehave. The errors could easily go undetected, so we better try to make it > work in the hook. > > Another reason is that the regexps for the commit message are case insensitive > (even though the errors there are more likely to be seen), so I thought that > would be consistent with this approach of being not completely strict. > > Do you think there's a chance that the case-insensitive regexp will match > something it shouldn't match? If not, what arguments do you see for regexp being > case-sensitive? Well the issue doesn't say the regexp here is matched case insensitively. Also the examples that contain both upper and lower-case imply that case sensitive matching will be used. Finally, unless I missed something, the documentation doesn't mention that the regexp here will be matched case insensitively either. I don't think it's too important which way we go but the documentation, examples and issue should reflect reality at least. https://codereview.adblockplus.org/29339623/diff/29340805/sitescripts/hg/READ... File sitescripts/hg/README.md (right): https://codereview.adblockplus.org/29339623/diff/29340805/sitescripts/hg/READ... sitescripts/hg/README.md:1: # Mercurial hooks Please could you add a brief introduction section that quickly explains the rough idea of sitescripts.hg? A sentence or two is fine, just to help someone who isn't familiar with the code decide if this is the module their looking for and to give a little context. Edit: The first two sentences from the module's docstring would work as a good introduction here. Maybe just copy them? https://codereview.adblockplus.org/29339623/diff/29340805/sitescripts/hg/READ... sitescripts/hg/README.md:21: Nit: Looks like a typo around "in"? https://codereview.adblockplus.org/29339623/diff/29340805/sitescripts/hg/READ... sitescripts/hg/README.md:44: interface of Trac and `hg.issue_url_template` as a template for producing links to Nit: Slightly too long line here and below https://codereview.adblockplus.org/29339623/diff/29340805/sitescripts/hg/READ... sitescripts/hg/README.md:54: What exactly does it mean when we say _`master` bookmark is passing a commit_. Nit: Maybe this sentence should end in a question mark? https://codereview.adblockplus.org/29339623/diff/29340805/sitescripts/hg/bin/... File sitescripts/hg/bin/update_issues.py (right): https://codereview.adblockplus.org/29339623/diff/29340805/sitescripts/hg/bin/... sitescripts/hg/bin/update_issues.py:52: updates.append('posted comment') Supposing there's a "feature branch" bookmark that in the repository containing several commits. Then we move the master bookmark forward to include those extra commits. If I understood the README right we would potentially perform a bunch of actions for those commits all at once? If so perhaps these messages need to provide more context, "posted comment" doesn't give much clue for which issue / commit a comment was posted. https://codereview.adblockplus.org/29339623/diff/29340805/sitescripts/hg/bin/... sitescripts/hg/bin/update_issues.py:104: milestones = filter(lambda m: not m['completed'], multicall()) Nit: I've been told to use list comprehension instead of filter before. https://codereview.adblockplus.org/29339623/diff/29340805/sitescripts/hg/bin/... sitescripts/hg/bin/update_issues.py:181: if kwargs['namespace'] != 'bookmarks': Nit: Replace these three with one if?
https://codereview.adblockplus.org/29339623/diff/29340805/sitescripts/hg/bin/... File sitescripts/hg/bin/update_issues.py (right): https://codereview.adblockplus.org/29339623/diff/29340805/sitescripts/hg/bin/... sitescripts/hg/bin/update_issues.py:34: @contextlib.contextmanager I'm learning some new tricks here :) https://codereview.adblockplus.org/29339623/diff/29340805/sitescripts/hg/bin/... sitescripts/hg/bin/update_issues.py:40: if getattr(exc, 'faultCode', 0) == 404: # Not found Nit: This comment doesn't really add anything. https://codereview.adblockplus.org/29339623/diff/29340805/sitescripts/hg/bin/... sitescripts/hg/bin/update_issues.py:83: ui.warn('warning: invalid module milestone regexp in config:' This message looks pretty clear but maybe we could make it clearer where the problem lies in the config? (And that the troublesome regexp was skipped.) For example: warning: Skipped invalid regexp for MILESTONE_NAME in [hg_module_milestones] config: Up to you anyway. https://codereview.adblockplus.org/29339623/diff/29340805/sitescripts/hg/bin/... sitescripts/hg/bin/update_issues.py:139: _update_issue(ui, config, issue_id, '', changes) Since comment seems like the only optional parameter for _update_issue perhaps it should be last (or a keyword parameter) and then it could simply be omitted here? https://codereview.adblockplus.org/29339623/diff/29340805/sitescripts/hg/bin/... sitescripts/hg/bin/update_issues.py:143: issue_number_regex = re.compile(r'\b(issue|fixes)\s+(\d+)\b', re.I) Maybe these regexps should be compiled as constants at the top? Currently they're being re-compiled each time _collect_references is called and it's being called in a loop.
Thanks for the comments, Dave. I pretty much agree with everything. https://codereview.adblockplus.org/29339623/diff/29339624/sitescripts/hg/bin/... File sitescripts/hg/bin/update_issues.py (right): https://codereview.adblockplus.org/29339623/diff/29339624/sitescripts/hg/bin/... sitescripts/hg/bin/update_issues.py:109: yield module, re.compile("^" + regexp + "$", re.I) On 2016/04/29 14:05:42, kzar wrote: > On 2016/04/24 21:49:50, Vasily Kuznetsov wrote: > > On 2016/04/18 14:48:30, kzar wrote: > > > I don't think this regexp should be case-insensitive. > > > > My reasoning was that we want to be more forgiving to people who configure the > > hook and create milestones, so that if they mess up the case, the hook won't > > misbehave. The errors could easily go undetected, so we better try to make it > > work in the hook. > > > > Another reason is that the regexps for the commit message are case insensitive > > (even though the errors there are more likely to be seen), so I thought that > > would be consistent with this approach of being not completely strict. > > > > Do you think there's a chance that the case-insensitive regexp will match > > something it shouldn't match? If not, what arguments do you see for regexp > being > > case-sensitive? > > Well the issue doesn't say the regexp here is matched case insensitively. Also > the examples that contain both upper and lower-case imply that case sensitive > matching will be used. Finally, unless I missed something, the documentation > doesn't mention that the regexp here will be matched case insensitively either. > > I don't think it's too important which way we go but the documentation, examples > and issue should reflect reality at least. I changed the README and the issue to reflect the fact that those regexps are matched case-insensitively. https://codereview.adblockplus.org/29339623/diff/29340805/sitescripts/hg/READ... File sitescripts/hg/README.md (right): https://codereview.adblockplus.org/29339623/diff/29340805/sitescripts/hg/READ... sitescripts/hg/README.md:1: # Mercurial hooks On 2016/04/29 14:05:43, kzar wrote: > Please could you add a brief introduction section that quickly explains the > rough idea of sitescripts.hg? A sentence or two is fine, just to help someone > who isn't familiar with the code decide if this is the module their looking for > and to give a little context. > > Edit: The first two sentences from the module's docstring would work as a good > introduction here. Maybe just copy them? If we're adding an introduction about sitescripts.hg, it makes sense to also at least mention the irchook. Unfortunately there's no documentation for it and not even a configuration example, so it's a bit hard to see what it's doing. From the name I guess that it posts commit references to IRC, so I left it at that for now. Let me know if you know more, I'd be happy to add it here. It probably should be a separate issue though. https://codereview.adblockplus.org/29339623/diff/29340805/sitescripts/hg/READ... sitescripts/hg/README.md:21: On 2016/04/29 14:05:42, kzar wrote: > Nit: Looks like a typo around "in"? Done. https://codereview.adblockplus.org/29339623/diff/29340805/sitescripts/hg/READ... sitescripts/hg/README.md:44: interface of Trac and `hg.issue_url_template` as a template for producing links to On 2016/04/29 14:05:43, kzar wrote: > Nit: Slightly too long line here and below Done. https://codereview.adblockplus.org/29339623/diff/29340805/sitescripts/hg/READ... sitescripts/hg/README.md:54: What exactly does it mean when we say _`master` bookmark is passing a commit_. On 2016/04/29 14:05:43, kzar wrote: > Nit: Maybe this sentence should end in a question mark? Done. https://codereview.adblockplus.org/29339623/diff/29340805/sitescripts/hg/bin/... File sitescripts/hg/bin/update_issues.py (right): https://codereview.adblockplus.org/29339623/diff/29340805/sitescripts/hg/bin/... sitescripts/hg/bin/update_issues.py:34: @contextlib.contextmanager On 2016/05/02 09:29:41, kzar wrote: > I'm learning some new tricks here :) Yeah, this is a pretty cool way to make context managers with less boilerplate code. And try/except/finally syntax is quite appropriate for what one usually wants to do with context managers. https://codereview.adblockplus.org/29339623/diff/29340805/sitescripts/hg/bin/... sitescripts/hg/bin/update_issues.py:40: if getattr(exc, 'faultCode', 0) == 404: # Not found On 2016/05/02 09:29:41, kzar wrote: > Nit: This comment doesn't really add anything. Fair enough, I'll remove it. https://codereview.adblockplus.org/29339623/diff/29340805/sitescripts/hg/bin/... sitescripts/hg/bin/update_issues.py:52: updates.append('posted comment') On 2016/04/29 14:05:43, kzar wrote: > Supposing there's a "feature branch" bookmark that in the repository containing > several commits. Then we move the master bookmark forward to include those extra > commits. If I understood the README right we would potentially perform a bunch > of actions for those commits all at once? > > If so perhaps these messages need to provide more context, "posted comment" > doesn't give much clue for which issue / commit a comment was posted. Unfortunately the commit messages didn't make it into this function before and the url of the commit is not terribly useful, so I had to cheat a little: - I changed `issue_commit_comment.tmpl` to include the commit message into the link (which makes it more useful too), and then - this function extracts the commit message and shows it to the user. So the output of hg push will look like this: updated https://issues.adblockplus.org/ticket/1308: - referenced a commit: Issue 1307, Fixes 1308, Issue 1309, Fixes 1311 - One updated https://issues.adblockplus.org/ticket/1309: - referenced a commit: Issue 1307, Fixes 1308, Issue 1309, Fixes 1311 - One - referenced a commit: Fixes 1309, Fixes 1310 - Two updated https://issues.adblockplus.org/ticket/1303: - set milestone to test-milestone-1.2 - closed updated https://issues.adblockplus.org/ticket/1304: - closed Done. https://codereview.adblockplus.org/29339623/diff/29340805/sitescripts/hg/bin/... sitescripts/hg/bin/update_issues.py:83: ui.warn('warning: invalid module milestone regexp in config:' On 2016/05/02 09:29:40, kzar wrote: > This message looks pretty clear but maybe we could make it clearer where the > problem lies in the config? (And that the troublesome regexp was skipped.) For > example: > > warning: Skipped invalid regexp for MILESTONE_NAME in [hg_module_milestones] > config: > > Up to you anyway. I suppose you meant "Skipped invalid regexp for MODULE_NAME...". Seems like a good idea to me. Done. https://codereview.adblockplus.org/29339623/diff/29340805/sitescripts/hg/bin/... sitescripts/hg/bin/update_issues.py:104: milestones = filter(lambda m: not m['completed'], multicall()) On 2016/04/29 14:05:43, kzar wrote: > Nit: I've been told to use list comprehension instead of filter before. Yep, it's definitely better here. I also renamed `multicall` to `get_milestones` to make the name less abstract. https://codereview.adblockplus.org/29339623/diff/29340805/sitescripts/hg/bin/... sitescripts/hg/bin/update_issues.py:139: _update_issue(ui, config, issue_id, '', changes) On 2016/05/02 09:29:40, kzar wrote: > Since comment seems like the only optional parameter for _update_issue perhaps > it should be last (or a keyword parameter) and then it could simply be omitted > here? Done. https://codereview.adblockplus.org/29339623/diff/29340805/sitescripts/hg/bin/... sitescripts/hg/bin/update_issues.py:143: issue_number_regex = re.compile(r'\b(issue|fixes)\s+(\d+)\b', re.I) On 2016/05/02 09:29:41, kzar wrote: > Maybe these regexps should be compiled as constants at the top? Currently > they're being re-compiled each time _collect_references is called and it's being > called in a loop. Actually it's not called in a loop, only once from each hook, but I agree that it's cleaner to move them out. Done. https://codereview.adblockplus.org/29339623/diff/29340805/sitescripts/hg/bin/... sitescripts/hg/bin/update_issues.py:181: if kwargs['namespace'] != 'bookmarks': On 2016/04/29 14:05:43, kzar wrote: > Nit: Replace these three with one if? Done.
https://codereview.adblockplus.org/29339623/diff/29340805/sitescripts/hg/READ... File sitescripts/hg/README.md (right): https://codereview.adblockplus.org/29339623/diff/29340805/sitescripts/hg/READ... sitescripts/hg/README.md:1: # Mercurial hooks On 2016/05/02 16:54:03, Vasily Kuznetsov wrote: > On 2016/04/29 14:05:43, kzar wrote: > > Please could you add a brief introduction section that quickly explains the > > rough idea of sitescripts.hg? A sentence or two is fine, just to help someone > > who isn't familiar with the code decide if this is the module their looking > for > > and to give a little context. > > > > Edit: The first two sentences from the module's docstring would work as a good > > introduction here. Maybe just copy them? > > If we're adding an introduction about sitescripts.hg, it makes sense to also at > least mention the irchook. Unfortunately there's no documentation for it and not > even a configuration example, so it's a bit hard to see what it's doing. From > the name I guess that it posts commit references to IRC, so I left it at that > for now. > > Let me know if you know more, I'd be happy to add it here. It probably should be > a separate issue though. You're right, this looks fine for now and at some point in the future hopefully we can add more documentation about the IRC integration. https://codereview.adblockplus.org/29339623/diff/29340805/sitescripts/hg/bin/... File sitescripts/hg/bin/update_issues.py (right): https://codereview.adblockplus.org/29339623/diff/29340805/sitescripts/hg/bin/... sitescripts/hg/bin/update_issues.py:52: updates.append('posted comment') On 2016/05/02 16:54:04, Vasily Kuznetsov wrote: > On 2016/04/29 14:05:43, kzar wrote: > > Supposing there's a "feature branch" bookmark that in the repository > containing > > several commits. Then we move the master bookmark forward to include those > extra > > commits. If I understood the README right we would potentially perform a bunch > > of actions for those commits all at once? > > > > If so perhaps these messages need to provide more context, "posted comment" > > doesn't give much clue for which issue / commit a comment was posted. > > Unfortunately the commit messages didn't make it into this function before and > the url of the commit is not terribly useful, so I had to cheat a little: > - I changed `issue_commit_comment.tmpl` to include the commit message into the > link (which makes it more useful too), and then > - this function extracts the commit message and shows it to the user. > > So the output of hg push will look like this: > > updated https://issues.adblockplus.org/ticket/1308: > - referenced a commit: Issue 1307, Fixes 1308, Issue 1309, Fixes 1311 - One > updated https://issues.adblockplus.org/ticket/1309: > - referenced a commit: Issue 1307, Fixes 1308, Issue 1309, Fixes 1311 - One > - referenced a commit: Fixes 1309, Fixes 1310 - Two > updated https://issues.adblockplus.org/ticket/1303: > - set milestone to test-milestone-1.2 > - closed > updated https://issues.adblockplus.org/ticket/1304: > - closed > > Done. Nice, looks good to me. Will mean I can even click the issue link straight from the console if I feel like it. https://codereview.adblockplus.org/29339623/diff/29340988/.sitescripts.example File .sitescripts.example (right): https://codereview.adblockplus.org/29339623/diff/29340988/.sitescripts.exampl... .sitescripts.example:159: platform=Adblock-Plus(-[\d\.]+)?-for-Chrome-Opera-Safari(-next)? Mind making these examples lowercase to avoid implying case sensitive matching? https://codereview.adblockplus.org/29339623/diff/29340988/sitescripts/hg/bin/... File sitescripts/hg/bin/update_issues.py (right): https://codereview.adblockplus.org/29339623/diff/29340988/sitescripts/hg/bin/... sitescripts/hg/bin/update_issues.py:51: def _update_issue(ui, config, issue_id, changes, comment=''): Nit: Seems weird to have a blank string mean there's no comment. Wouldn't None make more sense? (To me at least the blank string implies the comment is still being used even when omitted, but I don't think that's the case.) https://codereview.adblockplus.org/29339623/diff/29340988/sitescripts/hg/bin/... sitescripts/hg/bin/update_issues.py:60: updates.append(' - set milestone to {}'.format(changes['milestone'])) Nit: Seems inconsistent with above where you just append the two strings instead of using format?
https://codereview.adblockplus.org/29339623/diff/29340988/sitescripts/hg/READ... File sitescripts/hg/README.md (right): https://codereview.adblockplus.org/29339623/diff/29340988/sitescripts/hg/READ... sitescripts/hg/README.md:49: platform=Adblock-Plus(-[\d\.]+)?-for-Chrome-Opera-Safari(-next)? Mind making these regexp examples lower case too?
I addressed the comments on patch set 3. https://codereview.adblockplus.org/29339623/diff/29340988/.sitescripts.example File .sitescripts.example (right): https://codereview.adblockplus.org/29339623/diff/29340988/.sitescripts.exampl... .sitescripts.example:159: platform=Adblock-Plus(-[\d\.]+)?-for-Chrome-Opera-Safari(-next)? On 2016/05/03 06:47:35, kzar wrote: > Mind making these examples lowercase to avoid implying case sensitive matching? Done. https://codereview.adblockplus.org/29339623/diff/29340988/sitescripts/hg/READ... File sitescripts/hg/README.md (right): https://codereview.adblockplus.org/29339623/diff/29340988/sitescripts/hg/READ... sitescripts/hg/README.md:49: platform=Adblock-Plus(-[\d\.]+)?-for-Chrome-Opera-Safari(-next)? On 2016/05/03 07:28:22, kzar wrote: > Mind making these regexp examples lower case too? Done. https://codereview.adblockplus.org/29339623/diff/29340988/sitescripts/hg/bin/... File sitescripts/hg/bin/update_issues.py (right): https://codereview.adblockplus.org/29339623/diff/29340988/sitescripts/hg/bin/... sitescripts/hg/bin/update_issues.py:51: def _update_issue(ui, config, issue_id, changes, comment=''): On 2016/05/03 06:47:36, kzar wrote: > Nit: Seems weird to have a blank string mean there's no comment. Wouldn't None > make more sense? (To me at least the blank string implies the comment is still > being used even when omitted, but I don't think that's the case.) `comment` is passed to `ticket.update` at the end of this function and it seems that it expects an empty string if there's no comment. If you give it `None`, it breaks. https://codereview.adblockplus.org/29339623/diff/29340988/sitescripts/hg/bin/... sitescripts/hg/bin/update_issues.py:60: updates.append(' - set milestone to {}'.format(changes['milestone'])) On 2016/05/03 06:47:36, kzar wrote: > Nit: Seems inconsistent with above where you just append the two strings instead > of using format? Yep. The style guide now recommends using + in this case. Done.
LGTM https://codereview.adblockplus.org/29339623/diff/29340988/sitescripts/hg/bin/... File sitescripts/hg/bin/update_issues.py (right): https://codereview.adblockplus.org/29339623/diff/29340988/sitescripts/hg/bin/... sitescripts/hg/bin/update_issues.py:51: def _update_issue(ui, config, issue_id, changes, comment=''): On 2016/05/03 10:11:02, Vasily Kuznetsov wrote: > On 2016/05/03 06:47:36, kzar wrote: > > Nit: Seems weird to have a blank string mean there's no comment. Wouldn't None > > make more sense? (To me at least the blank string implies the comment is still > > being used even when omitted, but I don't think that's the case.) > > `comment` is passed to `ticket.update` at the end of this function and it seems > that it expects an empty string if there's no comment. If you give it `None`, it > breaks. Ah, missed that. Fair enough.
On 2016/05/03 10:13:49, kzar wrote: > LGTM > > https://codereview.adblockplus.org/29339623/diff/29340988/sitescripts/hg/bin/... > File sitescripts/hg/bin/update_issues.py (right): > > https://codereview.adblockplus.org/29339623/diff/29340988/sitescripts/hg/bin/... > sitescripts/hg/bin/update_issues.py:51: def _update_issue(ui, config, issue_id, > changes, comment=''): > On 2016/05/03 10:11:02, Vasily Kuznetsov wrote: > > On 2016/05/03 06:47:36, kzar wrote: > > > Nit: Seems weird to have a blank string mean there's no comment. Wouldn't > None > > > make more sense? (To me at least the blank string implies the comment is > still > > > being used even when omitted, but I don't think that's the case.) > > > > `comment` is passed to `ticket.update` at the end of this function and it > seems > > that it expects an empty string if there's no comment. If you give it `None`, > it > > breaks. > > Ah, missed that. Fair enough. Thank you, Dave. Felix, would you like to take a look at this review?
Looks mostly good to me. Just two nits. https://codereview.adblockplus.org/29339623/diff/29340994/.sitescripts.example File .sitescripts.example (right): https://codereview.adblockplus.org/29339623/diff/29340994/.sitescripts.exampl... .sitescripts.example:159: platform=adblock-plus(-[\d\.]+)?-for-chrome-opera-safari(-next)? Nit: "Platform" is supposed to be capitalized. https://codereview.adblockplus.org/29339623/diff/29340994/sitescripts/hg/bin/... File sitescripts/hg/bin/update_issues.py (right): https://codereview.adblockplus.org/29339623/diff/29340994/sitescripts/hg/bin/... sitescripts/hg/bin/update_issues.py:87: yield module, re.compile('^' + regexp + '$', re.I) Don't use the plus operator when concatenating more than two strings, as per our style guide. Use '^{}$'.format(regexp) instead.
https://codereview.adblockplus.org/29339623/diff/29340994/.sitescripts.example File .sitescripts.example (right): https://codereview.adblockplus.org/29339623/diff/29340994/.sitescripts.exampl... .sitescripts.example:159: platform=adblock-plus(-[\d\.]+)?-for-chrome-opera-safari(-next)? On 2016/05/17 09:10:57, Sebastian Noack wrote: > Nit: "Platform" is supposed to be capitalized. Done. https://codereview.adblockplus.org/29339623/diff/29340994/sitescripts/hg/bin/... File sitescripts/hg/bin/update_issues.py (right): https://codereview.adblockplus.org/29339623/diff/29340994/sitescripts/hg/bin/... sitescripts/hg/bin/update_issues.py:87: yield module, re.compile('^' + regexp + '$', re.I) On 2016/05/17 09:10:57, Sebastian Noack wrote: > Don't use the plus operator when concatenating more than two strings, as per our > style guide. Use '^{}$'.format(regexp) instead. Done.
LGTM
Did more of a high-level review, so I didn't notice any issue that matters. Mostly wording stuff, one code style thing. https://codereview.adblockplus.org/29339623/diff/29341464/sitescripts/hg/READ... File sitescripts/hg/README.md (right): https://codereview.adblockplus.org/29339623/diff/29341464/sitescripts/hg/READ... sitescripts/hg/README.md:15: issues in Adblock Plus issue tracker. Nit: "in the" maybe? https://codereview.adblockplus.org/29339623/diff/29341464/sitescripts/hg/READ... sitescripts/hg/README.md:26: on their module) when `master` bookmark passes over the commits that fix Nit: "when the"? https://codereview.adblockplus.org/29339623/diff/29341464/sitescripts/hg/READ... sitescripts/hg/README.md:59: expression of issue's module will be assigned to the issue when the `master` Nit: "of an"? https://codereview.adblockplus.org/29339623/diff/29341464/sitescripts/hg/READ... sitescripts/hg/README.md:66: those changes in our working copy when we do `hg checkout master`. Nit: `checkout` is just an alias for `update`, which is the normal command to use. Technically it's correct though, so I don't mind much. https://codereview.adblockplus.org/29339623/diff/29341464/sitescripts/hg/bin/... File sitescripts/hg/bin/update_issues.py (right): https://codereview.adblockplus.org/29339623/diff/29341464/sitescripts/hg/bin/... sitescripts/hg/bin/update_issues.py:19: are pushed and and "master" bookmark is moved. See README.md for more Nit: One "and" should do :) Also, "the master bookmark" I guess. https://codereview.adblockplus.org/29339623/diff/29341464/sitescripts/hg/bin/... sitescripts/hg/bin/update_issues.py:32: _IssueRef = collections.namedtuple('IssueRef', 'id commits is_fixed') AFAIK we mostly use double quotes (same as in JS where the two are also functionally equivalent), I think we should stick to that consistently in new code.
https://codereview.adblockplus.org/29339623/diff/29341464/sitescripts/hg/bin/... File sitescripts/hg/bin/update_issues.py (right): https://codereview.adblockplus.org/29339623/diff/29341464/sitescripts/hg/bin/... sitescripts/hg/bin/update_issues.py:32: _IssueRef = collections.namedtuple('IssueRef', 'id commits is_fixed') On 2016/05/17 20:12:06, Felix Dahlke wrote: > AFAIK we mostly use double quotes (same as in JS where the two are also > functionally equivalent), I think we should stick to that consistently in new > code. We use single quotes consistently in new Python code (except when avoiding escaping of embedded quotes). Frankly I did that ever since except when aiming to be consistent with surrounding legacy code. But it's in the coding style now. Also keep in mind that this matches the behaviour of repr() i.e. the natural representation of literals in Python.
Alright, so no comments about code at all, just some wording things. LGTM with those addressed. https://codereview.adblockplus.org/29339623/diff/29341464/sitescripts/hg/bin/... File sitescripts/hg/bin/update_issues.py (right): https://codereview.adblockplus.org/29339623/diff/29341464/sitescripts/hg/bin/... sitescripts/hg/bin/update_issues.py:32: _IssueRef = collections.namedtuple('IssueRef', 'id commits is_fixed') On 2016/05/17 20:39:39, Sebastian Noack wrote: > On 2016/05/17 20:12:06, Felix Dahlke wrote: > > AFAIK we mostly use double quotes (same as in JS where the two are also > > functionally equivalent), I think we should stick to that consistently in new > > code. > > We use single quotes consistently in new Python code (except when avoiding > escaping of embedded quotes). Frankly I did that ever since except when aiming > to be consistent with surrounding legacy code. But it's in the coding style now. > Also keep in mind that this matches the behaviour of repr() i.e. the natural > representation of literals in Python. I see. Makes more sense to me to do this consistently across languages, but it doesn't matter much. From some quick grepping, it looks like around 60% of our Python code uses single quotes, so it's the more popular thing already. Had a quick look at some code in cpython to see what they use. Seems totally inconsistent. Guess that explains why PEP-8 doesn't specify it :D
https://codereview.adblockplus.org/29339623/diff/29341464/sitescripts/hg/bin/... File sitescripts/hg/bin/update_issues.py (right): https://codereview.adblockplus.org/29339623/diff/29341464/sitescripts/hg/bin/... sitescripts/hg/bin/update_issues.py:32: _IssueRef = collections.namedtuple('IssueRef', 'id commits is_fixed') On 2016/05/17 21:24:08, Felix Dahlke wrote: > On 2016/05/17 20:39:39, Sebastian Noack wrote: > > On 2016/05/17 20:12:06, Felix Dahlke wrote: > > > AFAIK we mostly use double quotes (same as in JS where the two are also > > > functionally equivalent), I think we should stick to that consistently in > new > > > code. > > > > We use single quotes consistently in new Python code (except when avoiding > > escaping of embedded quotes). Frankly I did that ever since except when aiming > > to be consistent with surrounding legacy code. But it's in the coding style > now. > > Also keep in mind that this matches the behaviour of repr() i.e. the natural > > representation of literals in Python. > > I see. Makes more sense to me to do this consistently across languages, but it > doesn't matter much. From some quick grepping, it looks like around 60% of our > Python code uses single quotes, so it's the more popular thing already. > > Had a quick look at some code in cpython to see what they use. Seems totally > inconsistent. Guess that explains why PEP-8 doesn't specify it :D I believe that PEP-8 doesn't dictates which quotes to use simply because it isn't too important, and enforcing it could alienate some adopters. However, still using single quotes is more natural in Python as this is what the repl and built-in APIs do when preparing strings to be displayed. Also, most Python documentation uses single quotes. While its exactly the other way around in JavaScript, where browsers use double quotes when displaying strings in the console, and most JavaScript documentation uses double quotes as well. So there might actually be a reason to do it differently in different languages, in order to stick to what's common in the context of those individual languages.
https://codereview.adblockplus.org/29339623/diff/29341464/sitescripts/hg/READ... File sitescripts/hg/README.md (right): https://codereview.adblockplus.org/29339623/diff/29341464/sitescripts/hg/READ... sitescripts/hg/README.md:15: issues in Adblock Plus issue tracker. On 2016/05/17 20:12:05, Felix Dahlke wrote: > Nit: "in the" maybe? Done. https://codereview.adblockplus.org/29339623/diff/29341464/sitescripts/hg/READ... sitescripts/hg/README.md:26: on their module) when `master` bookmark passes over the commits that fix On 2016/05/17 20:12:06, Felix Dahlke wrote: > Nit: "when the"? Done. https://codereview.adblockplus.org/29339623/diff/29341464/sitescripts/hg/READ... sitescripts/hg/README.md:59: expression of issue's module will be assigned to the issue when the `master` On 2016/05/17 20:12:06, Felix Dahlke wrote: > Nit: "of an"? Should not it be "of the issue's module"? We're talking about specific issue and module here, right? I changed it to this. https://codereview.adblockplus.org/29339623/diff/29341464/sitescripts/hg/READ... sitescripts/hg/README.md:66: those changes in our working copy when we do `hg checkout master`. On 2016/05/17 20:12:05, Felix Dahlke wrote: > Nit: `checkout` is just an alias for `update`, which is the normal command to > use. Technically it's correct though, so I don't mind much. I think I wrote it this way thinking about a universal command that git users would also understand. Perhaps it doesn't make much sense since we're talking about a mercurial hook, but I have a mild preference for this more inclusive approach. So I'll leave it as is for now. https://codereview.adblockplus.org/29339623/diff/29341464/sitescripts/hg/bin/... File sitescripts/hg/bin/update_issues.py (right): https://codereview.adblockplus.org/29339623/diff/29341464/sitescripts/hg/bin/... sitescripts/hg/bin/update_issues.py:19: are pushed and and "master" bookmark is moved. See README.md for more On 2016/05/17 20:12:06, Felix Dahlke wrote: > Nit: One "and" should do :) Also, "the master bookmark" I guess. Done :) Also changed the quotes around "master" to be consistent with how it's written in the README. https://codereview.adblockplus.org/29339623/diff/29341464/sitescripts/hg/bin/... sitescripts/hg/bin/update_issues.py:32: _IssueRef = collections.namedtuple('IssueRef', 'id commits is_fixed') On 2016/05/18 06:52:39, Sebastian Noack wrote: > On 2016/05/17 21:24:08, Felix Dahlke wrote: > > On 2016/05/17 20:39:39, Sebastian Noack wrote: > > > On 2016/05/17 20:12:06, Felix Dahlke wrote: > > > > AFAIK we mostly use double quotes (same as in JS where the two are also > > > > functionally equivalent), I think we should stick to that consistently in > > new > > > > code. > > > > > > We use single quotes consistently in new Python code (except when avoiding > > > escaping of embedded quotes). Frankly I did that ever since except when > aiming > > > to be consistent with surrounding legacy code. But it's in the coding style > > now. > > > Also keep in mind that this matches the behaviour of repr() i.e. the natural > > > representation of literals in Python. > > > > I see. Makes more sense to me to do this consistently across languages, but it > > doesn't matter much. From some quick grepping, it looks like around 60% of our > > Python code uses single quotes, so it's the more popular thing already. > > > > Had a quick look at some code in cpython to see what they use. Seems totally > > inconsistent. Guess that explains why PEP-8 doesn't specify it :D > > I believe that PEP-8 doesn't dictates which quotes to use simply because it > isn't too important, and enforcing it could alienate some adopters. However, > still using single quotes is more natural in Python as this is what the repl and > built-in APIs do when preparing strings to be displayed. Also, most Python > documentation uses single quotes. While its exactly the other way around in > JavaScript, where browsers use double quotes when displaying strings in the > console, and most JavaScript documentation uses double quotes as well. So there > might actually be a reason to do it differently in different languages, in order > to stick to what's common in the context of those individual languages. Fully agree with Sebastian here. Regarding JavaScript, I would also add that JSON requires double quotes so there's another incentive to use double quotes everywhere for consistency. So given that there's some, rather remote, but still, similarity between `JSON.stringify` and `repr`, we're sort of doing the same thing in our approach to Python strings. Anyway, I like how our style guide is quite definite about the quotes and (bonus!) we even have flake8-abp that automatically enforces it.
Good arguments for the single quotes, but I was OK with it anyway :) LGTM https://codereview.adblockplus.org/29339623/diff/29341464/sitescripts/hg/READ... File sitescripts/hg/README.md (right): https://codereview.adblockplus.org/29339623/diff/29341464/sitescripts/hg/READ... sitescripts/hg/README.md:59: expression of issue's module will be assigned to the issue when the `master` On 2016/05/18 08:30:44, Vasily Kuznetsov wrote: > On 2016/05/17 20:12:06, Felix Dahlke wrote: > > Nit: "of an"? > > Should not it be "of the issue's module"? We're talking about specific issue and > module here, right? I changed it to this. Acknowledged. https://codereview.adblockplus.org/29339623/diff/29341464/sitescripts/hg/READ... sitescripts/hg/README.md:66: those changes in our working copy when we do `hg checkout master`. On 2016/05/18 08:30:44, Vasily Kuznetsov wrote: > On 2016/05/17 20:12:05, Felix Dahlke wrote: > > Nit: `checkout` is just an alias for `update`, which is the normal command to > > use. Technically it's correct though, so I don't mind much. > > I think I wrote it this way thinking about a universal command that git users > would also understand. Perhaps it doesn't make much sense since we're talking > about a mercurial hook, but I have a mild preference for this more inclusive > approach. So I'll leave it as is for now. Acknowledged. 
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
