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

[Thomas De Schampheleire]

El vie., 23 nov. 2018 a las 12:49, Mads Kiilerich
(<mads at kiilerich.com>) escribió:
>
> 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.

Ok, I see what you mean now.
When extracting that code I actually only wanted to let it be shared
between changeset and pullrequest controller. But calling a controller
method from another controller seems not possible, so mostly for
practical reasons I placed it outside the class without much further
thought.

The refactoring that you describe sounds very good with clear benefits
in understandability and testability, but is a bigger goal spanning
the entire codebase.
I would thus leave that for later, putting first priority at getting
out the next release, then finishing SSH support to get ready for a
1.0 release.

>
> >>> @@ -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".

I don't mind the explicit 'is not None', I'm fine with using it consistently.
But, someone else told me it is more pythonic to use 'if foo' rather
than 'if foo is not None'. I was looking for a reference but couldn't
find it. PEP8 just warns about being aware of the difference, but does
not seem to make a clear judgement.

>
> >>
> >>> @@ -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?

I really don't get you.
Here is the final code as it is in the changeset controller, basically
a move from what was in pullrequest after aligning:

    if pull_request:
        action = 'user_commented_pull_request:%s' % pull_request_id
    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,
                      c.db_repo, request.ip_addr)

There are two action logger calls. First is for the commenting part,
and differentiates revision and pull request.
Second action_logger call is only there for closing of pull requests,
and uses the 'user_closed_pull_request' string.

What is not correct here?

Thanks,
Thomas