On 11/22/2018 09:25 PM, Thomas De Schampheleire wrote:
El mié., 21 nov. 2018 a las 12:48, Mads Kiilerich
(<m...@kiilerich.com>) escribió:
On 11/20/2018 09:32 PM, Thomas De Schampheleire wrote:
# HG changeset patch
# User Thomas De Schampheleire <thomas.de_schamphele...@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
_______________________________________________
kallithea-general mailing list
kallithea-general@sfconservancy.org
https://lists.sfconservancy.org/mailman/listinfo/kallithea-general
