Issue 14103044: tryton-tools: Add reviewbot (Closed)
DescriptionThis is a python script that submits on rietveld instances the result of flake8 runs.
Patch Set 1 #
Total comments: 19
Patch Set 2 : do not send LGTM / guess version and module url #
Total comments: 1
Patch Set 3 : Add comment when everything is fine #MessagesTotal messages: 10
The bot could go into tryton-tools https://codereview.appspot.com/14103044/diff/1/reviewbot.py File reviewbot.py (right): https://codereview.appspot.com/14103044/diff/1/reviewbot.py#newcode20 reviewbot.py:20: REPOSITORIES = { Why not construct the url base on repo name? https://codereview.appspot.com/14103044/diff/1/reviewbot.py#newcode25 reviewbot.py:25: DB_PATH = os.path.expanduser('~/reviewbot.db') ~/.reviewbot.db https://codereview.appspot.com/14103044/diff/1/reviewbot.py#newcode58 reviewbot.py:58: self.options.ignore = ('E122', 'E123', 'E124', 'E126', 'E128') normally you can configure flake8 with .config/flake8 https://codereview.appspot.com/14103044/diff/1/reviewbot.py#newcode133 reviewbot.py:133: hgreview.review(my_ui, repo, **{ Why **{} instead of clean=False, … https://codereview.appspot.com/14103044/diff/1/reviewbot.py#newcode214 reviewbot.py:214: 'Not LGTM: patch is not applicable on trunk') For me, bot should not pronounce on validity of the patch (never use LGTM). https://codereview.appspot.com/14103044/diff/1/reviewbot.py#newcode222 reviewbot.py:222: 'Not LGTM: flake8 errors') idem
Sign in to reply to this message.
https://codereview.appspot.com/14103044/diff/1/reviewbot.py File reviewbot.py (right): https://codereview.appspot.com/14103044/diff/1/reviewbot.py#newcode58 reviewbot.py:58: self.options.ignore = ('E122', 'E123', 'E124', 'E126', 'E128') Add E711 since python-sql requires to write "foo == None" ? https://codereview.appspot.com/14103044/diff/1/reviewbot.py#newcode214 reviewbot.py:214: 'Not LGTM: patch is not applicable on trunk') On 2013/09/30 13:41:42, ced wrote: > For me, bot should not pronounce on validity of the patch (never use LGTM). A warning is still a good thing though
Sign in to reply to this message.
https://codereview.appspot.com/14103044/diff/1/reviewbot.py File reviewbot.py (right): https://codereview.appspot.com/14103044/diff/1/reviewbot.py#newcode214 reviewbot.py:214: 'Not LGTM: patch is not applicable on trunk') On 2013/09/30 14:52:12, jean.cavallo wrote: > On 2013/09/30 13:41:42, ced wrote: > > For me, bot should not pronounce on validity of the patch (never use LGTM). > > A warning is still a good thing though This should just be informational, as it could be desired to propose a patch for released versions (e.g. not each patch is backportable). https://codereview.appspot.com/14103044/diff/1/reviewbot.py#newcode222 reviewbot.py:222: 'Not LGTM: flake8 errors') On 2013/09/30 13:41:42, ced wrote: > idem +1 + a prefix, that this is the result of some automatic checking
Sign in to reply to this message.
https://codereview.appspot.com/14103044/diff/1/reviewbot.py File reviewbot.py (right): https://codereview.appspot.com/14103044/diff/1/reviewbot.py#newcode20 reviewbot.py:20: REPOSITORIES = { On 2013/09/30 13:41:42, ced wrote: > Why not construct the url base on repo name? Done. https://codereview.appspot.com/14103044/diff/1/reviewbot.py#newcode25 reviewbot.py:25: DB_PATH = os.path.expanduser('~/reviewbot.db') On 2013/09/30 13:41:42, ced wrote: > ~/.reviewbot.db Done. https://codereview.appspot.com/14103044/diff/1/reviewbot.py#newcode58 reviewbot.py:58: self.options.ignore = ('E122', 'E123', 'E124', 'E126', 'E128') On 2013/09/30 14:52:12, jean.cavallo wrote: > Add E711 since python-sql requires to write "foo == None" ? Done. https://codereview.appspot.com/14103044/diff/1/reviewbot.py#newcode133 reviewbot.py:133: hgreview.review(my_ui, repo, **{ On 2013/09/30 13:41:42, ced wrote: > Why **{} instead of clean=False, … Done. https://codereview.appspot.com/14103044/diff/1/reviewbot.py#newcode214 reviewbot.py:214: 'Not LGTM: patch is not applicable on trunk') On 2013/09/30 13:41:42, ced wrote: > For me, bot should not pronounce on validity of the patch (never use LGTM). Done. https://codereview.appspot.com/14103044/diff/1/reviewbot.py#newcode214 reviewbot.py:214: 'Not LGTM: patch is not applicable on trunk') On 2013/09/30 14:52:12, jean.cavallo wrote: > On 2013/09/30 13:41:42, ced wrote: > > For me, bot should not pronounce on validity of the patch (never use LGTM). > > A warning is still a good thing though Done. https://codereview.appspot.com/14103044/diff/1/reviewbot.py#newcode214 reviewbot.py:214: 'Not LGTM: patch is not applicable on trunk') On 2013/09/30 16:15:52, yangoon wrote: > On 2013/09/30 14:52:12, jean.cavallo wrote: > > On 2013/09/30 13:41:42, ced wrote: > > > For me, bot should not pronounce on validity of the patch (never use LGTM). > > > > A warning is still a good thing though > > This should just be informational, as it could be desired to propose a patch > for released versions (e.g. not each patch is backportable). I added support for released version using the following schema: module_name version: description https://codereview.appspot.com/14103044/diff/1/reviewbot.py#newcode222 reviewbot.py:222: 'Not LGTM: flake8 errors') On 2013/09/30 13:41:42, ced wrote: > idem Done. https://codereview.appspot.com/14103044/diff/1/reviewbot.py#newcode222 reviewbot.py:222: 'Not LGTM: flake8 errors') On 2013/09/30 16:15:52, yangoon wrote: > On 2013/09/30 13:41:42, ced wrote: > > idem > > +1 + a prefix, that this is the result of some automatic checking The name of the reviewer is going to be 'tryton.reviewbot' (I registered this user on gmail).
Sign in to reply to this message.
LGTM https://codereview.appspot.com/14103044/diff/9001/reviewbot.py File reviewbot.py (right): https://codereview.appspot.com/14103044/diff/9001/reviewbot.py#newcode223 reviewbot.py:223: finalize_comments(session, issue_id, 'flake8 errors') Why not: "guide lines" or even nothing.
Sign in to reply to this message.
|
