[PATCH 12 of 14] controllers: align pullrequests.comment with changeset.comment

[Mads Kiilerich]

On 11/22/2018 09:25 PM, Thomas De Schampheleire wrote:
> El mié., 21 nov. 2018 a las 12:48, Mads Kiilerich
> (<mads at kiilerich.com>) escribió:
>> On 11/20/2018 09:32 PM, Thomas De Schampheleire wrote:
>>> # HG changeset patch
>>> # User Thomas De Schampheleire <thomas.de_schampheleire at nokia.com>
>>> # Date 1542402570 -3600
>>> #      Fri Nov 16 22:09:30 2018 +0100
>>> # Node ID 4e6bfd3650ddfaf9d2bfbb166eaaec15f5194135
>>> # Parent  abef10c5c0d21af567d76d6ffaf5356cc3c3be81
>>> controllers: align pullrequests.comment with changeset.comment
>>>
>>> This commit purely serves to highlight the differences.
>>> The subsequent commit will remove the duplication.
>>>
>>> diff --git a/kallithea/controllers/changeset.py b/kallithea/controllers/changeset.py
>>> --- a/kallithea/controllers/changeset.py
>>> +++ b/kallithea/controllers/changeset.py
>>> @@ -365,48 +365,87 @@ class ChangesetController(BaseRepoContro
>>>        @LoginRequired()
>>>        @HasRepoPermissionLevelDecorator('read')
>>>        @jsonify
>>> -    def comment(self, repo_name, revision):
>>> +    def comment(self, repo_name, revision, pull_request_id=None, allowed_to_change_status=True):
>> (When extracting this as function later on, it would be nice to get a
>> docstring.)
> Ok. I added something but not sure if it's the type of docstring you
> expect. Feel free to change it.

Thanks.

My thoughts are something like this:

A method is of course not a function - it works on the instance state. 
Our controller methods do however not really use the instance - the 
instances are mainly used as a namespace. The controller methods do 
however use the ugly global thread local variables like "request". But 
ok, it doesn't matter much for the casual observer if the state is in 
the instance or in a global variable.

I think it gets more tricky when we extract functionality to 
(apparently) pure functions. It is more surprising that they actually 
work on a state. That needs clear documentation.

It would be nice if we could make it so we only access the global state 
in (meta) controllers, and then let them pass all values as parameters 
to the rest of the world.
That also imply that we should separate our lib into low level pure 
functions that can be unit tested and doesn't have tg dependencies, and 
more high level functions. That would probably also help us get rid of 
some dependency cycles.

>>> @@ -648,10 +652,10 @@ class PullrequestsController(BaseRepoCon
>>>            allowed_to_change_status = self._is_allowed_to_change_status(pull_request)
>>>            if not allowed_to_change_status:
>>>                if status or close_pr:
>>> -                h.flash(_('No permission to change pull request status'), 'error')
>>> +                h.flash(_('No permission to change status'), 'error')
>>>                    raise HTTPForbidden()
>>>
>>> -        if delete == "delete":
>>> +        if pull_request and delete == "delete":
>> (Generally, I prefer to use the slightly more verbose "is not None"
>> instead of relying on falsiness of None ... and thus the assumption that
>> objects never are "false" in other ways. But I'm also lazy and would
>> perhaps do it this way ...)
> For pull request ids, 'is not None' is needed because '0' could be a
> valid pull request id. But 'pull_request' is the object from the
> model, so either it exists or it does not.
> I'm fine adding 'is not None' explicitly, but I think we should then
> add this guideline to our coding rules in the manual.
> Let me know your conclusion...

I agree the "is not None" is redundant if looking at is as a C-ish 
nullable pointer.

In Python, it also depends on the object not defining __nonzero__ or 
__len__. I don't know how likely it is that "random" classes implement 
these, but a thorough review of the code will have to consider it. I 
thus prefer the "Explicit is better than implicit" 'is not None' so we 
can reason about the code with a minimum amount of assumptions .

I don't know if it deserves to be in the guidelines. We currently don't 
follow it consistently, and it could perhaps be considered a part of 
"pythonic best practice".

>>
>>> @@ -672,26 +676,30 @@ class PullrequestsController(BaseRepoCon
>>>            comment = create_comment(
>>>                text,
>>>                status,
>>> +            revision=revision,
>>>                pull_request_id=pull_request_id,
>>>                f_path=f_path,
>>>                line_no=line_no,
>>>                closing_pr=close_pr,
>>>            )
>>>
>>> -        action_logger(request.authuser,
>>> -                      'user_commented_pull_request:%s' % pull_request_id,
>>> -                      c.db_repo, request.ip_addr)
>>> -
>>>            if status:
>>>                ChangesetStatusModel().set_status(
>>>                    c.db_repo.repo_id,
>>>                    status,
>>>                    request.authuser.user_id,
>>>                    comment,
>>> -                pull_request=pull_request_id
>>> +                revision=revision,
>>> +                pull_request=pull_request_id,
>>>                )
>>>
>>> -        if close_pr:
>>> +        if pull_request:
>>> +            action = 'user_closed_pull_request:%s' % pull_request_id
>> 'user_commented_pull_request' ... so apparently no coverage of this
>> logging ... but no big deal ...
> Here I'm not sure what you mean: this log string is effectively for
> closing the PR so the text seems correct?

The check for close_pr comes next and has its own action logger?

>>> +        else:
>>> +            action = 'user_commented_revision:%s' % revision
>>> +        action_logger(request.authuser, action, c.db_repo, request.ip_addr)
>>> +
>>> +        if pull_request and close_pr:
>>>                PullRequestModel().close_pull_request(pull_request_id)
>>>                action_logger(request.authuser,
>>>                              'user_closed_pull_request:%s' % pull_request_id,

/Mads