new comments framework

[lethliel]

List and create comments

adding comments command to osc2. Comments can be listed, created, replied to and deleted.

Base Syntax:

osc2 comments <action> package api://project/package [-c] [-p]
osc2 comments <action> project api://project [-c] [-p]
osc2 comments <action> request api://reqeust[-c] [-p]

where action can be list or create
-c defines the comment text.
If -p is given the comment is a reply to an existing comment with the given id

The list command has an interactive mode:
osc2 comment list package -i api://project/package

In this mode you can go through the comments and can:

• n (next comment)
• reply (reply to comment)
• create (create new comment)
• delete (delete comment)

Delete comments

You can delete a comment:

osc2 comment delete api://comment_id

[marcus-h]

On 2017-02-28 00:01:38 -0800, Marco Strigl wrote: In this mode you can go through the comments and can: * n (next comment)

Let's stick to full names for now (edit: ok, the code uses the full name) Some comments below. For now I skipped the whole interactive/shell code and more or less neglected the ui.py and jinja2 files.

diff --git a/osc2/cli/comment/comment.py b/osc2/cli/comment/comment.py new file mode 100644 index 0000000..48ac54c --- /dev/null +++ b/osc2/cli/comment/comment.py @@ -0,0 +1,199 @@ +"""Provides controller functions for the "comment" command.""" + +import logging

Unused? (see below)

+import collections + +from osc2.httprequest import HTTPError +from osc2.util.xpath import XPathBuilder +from osc2.comments import PackageComment, ProjectComment, RequestComment +from osc2.comments import DeleteComment

Just use a single from ... import ...: from osc2.comments import (PackageComment, ProjectComment, RequestComment, DeleteComment)

+from osc2.cli.util.env import run_pager, edit_message

Unused import?

+from osc2.cli.util.shell import AbstractShell, ShellSyntaxError +

Minor: style: additional newline

+LIST_TEMPLATE = 'comment/comment_list.jinja2' +SHELL_TEMPLATE = 'comment/comment_shell.jinja2' +CREATE_TEMPLATE = 'comment/comment_create.jinja2' + + +def logger(): + """Returns a logging.Logger object.""" + return logging.getLogger(__name__) +

Unused.

+ +class CommentController(object): + """ Main class for the comment handling. Includes + methods for package, project and request comments + + """ +

Convention: style: multi line doc strings have the following format: """Short summary/description. Detailed description. """ (Note the empty lines)

+ @classmethod + def package_comment(cls, renderer, project, package, method, info, + comment=None, parent=None): + """Method for package command handling""" + global LIST_TEMPLATE + query = {'apiurl': info.apiurl} + pkg_comment_ctrl = PackageComment(project, package)

Hmm this is a misnomer: a PackageComment instance is no "controller".

+ result = getattr(pkg_comment_ctrl, method)(comment, parent, **query) + if method == 'list': + formated_comments = _format_for_output(result)

"Typo": formatted_comments = ... (other uses for "formated_comments" need to be adapted as well)

+ if info.interactive: + cls.shell(renderer, info.shell_cls, formated_comments, + pkg_comment_ctrl, info) + renderer.render(LIST_TEMPLATE, comments=formated_comments) + else: + renderer.render(CREATE_TEMPLATE, status=result) +

Convention: declare CREATE_TEMPLATE as global at the beginning of this method. (This also applies to the following methods.)

+ @classmethod + def project_comment(cls, renderer, project, method, info, comment=None, + parent=None): + """Method for project command handling""" + global LIST_TEMPLATE + query = {'apiurl': info.apiurl} + proj_comment_ctrl = ProjectComment(project) + result = getattr(proj_comment_ctrl, method)(comment, parent, **query) + if method == 'list': + formated_comments = _format_for_output(result) + if info.interactive: + cls.shell(renderer, info.shell_cls, formated_comments, + proj_comment_ctrl, info) + renderer.render(LIST_TEMPLATE, comments=formated_comments) + else: + renderer.render(CREATE_TEMPLATE, status=result) + + @classmethod + def request_comment(cls, renderer, request, method, info, comment=None, + parent=None): + """Method for request command handling""" + global LIST_TEMPLATE + query = {'apiurl': info.apiurl} + req_comment_ctrl = RequestComment(request) + result = getattr(req_comment_ctrl, method)(comment, parent, **query) + if method == 'list': + formated_comments = _format_for_output(result) + if info.interactive: + cls.shell(renderer, info.shell_cls, formated_comments, + req_comment_ctrl, info) + renderer.render(LIST_TEMPLATE, comments=formated_comments) + else: + renderer.render(CREATE_TEMPLATE, status=result) +

All 3 methods basically do the same. We should stick to the DRY-principle and unify them. For instance (untested), @classmethod def comments_list(cls, kind, renderer, info): global LIST_TEMPLATE args = cls._get_args(info) comments = comments_list(kind, *args) formatted_comments = cls._format_for_output(comments) if opts.interactive: ... else: renderer.render(LIST_TEMPLATE, comments=formatted_comments) @classmethod def comment_create(cls, kind, renderer, info): global CREATE_DELETE_TEMPLATE args = cls._get_args(info) comment = info.comment if comment is None: comment = edit_message() status = comment_create(kind, comment, parent=info.parent, *args) renderer.render(CREATE_DELETE_TEMPLATE, status=status) @classmethod def comment_delete(cls, kind, comment_id, renderer, info): global CREATE_DELETE_TEMPLATE status = comment_delete(kind, comment_id) renderer.render(CREATE_DELETE_TEMPLATE, status=status) @classmethod def _get_args(cls, info): if 'package' in info: return [info.project, info.package] elif 'project' in info: return [info.project] else: return [info.request]

+def _format_for_output(result): + f_comments = collections.OrderedDict() + level = 0 + """Order comments and write to an ordered dict""" + comments = [(c, 0) for c in ***@***.***)]')] + while comments: + comment, level = comments.pop(0) + xpb = XPathBuilder(context_item=True) + xp = xpb.comment[xpb.attr('parent') == comment.get('id')] + children = result.xpath(xp.tostring()) + f_comments[comment.get('id')] = {'who': comment.get('who'), + 'when': comment.get('when'), + 'comment': comment, + 'level': level + }

Minor: style: the closing brace should be aligned with the last key.

+ for child in children: + comments[:0] = [(child, level + 1)] + return f_comments

Hmm is there a specific reason why this function is no class method? (Hmm maybe we should also make all class methods module functions...)

diff --git a/osc2/cli/comment/comment_list.jinja2 b/osc2/cli/comment/comment_list.jinja2 new file mode 100644 index 0000000..4d97221 --- /dev/null +++ b/osc2/cli/comment/comment_list.jinja2 @@ -0,0 +1,12 @@ +{% for cid, com in comments.items() %} + {%- if (com.get('level') == 0) %} + {{- '%s' | format( '+' * 60) }}

Leading whitespace in format call.

+ {%- endif %} + {{ '%s (%s) from %s at %s:' | format( ' ' * com.get('level'),

Leading whitespace in format call.

+ cid, + com.get('who'), + com.get('when'))

Indentation should be adjusted (one space less).

+ }} + {{ '%s %s' | format(' ' * com.get('level'), com.get('comment')) }} + {% if true %} {% endif %}

Is this needed?

diff --git a/osc2/cli/comment/comment_shell.jinja2 b/osc2/cli/comment/comment_shell.jinja2 new file mode 100644 index 0000000..68b66ad --- /dev/null +++ b/osc2/cli/comment/comment_shell.jinja2 @@ -0,0 +1,10 @@ +{% if (com.get('level') == 0) %} +{{ '%s' | format( '+' * 60) }}

Whitespace (see above)

+{% endif %} +{{ '%s (%s) from %s at %s:' | format( ' ' * com.get('level'), + cid, + com.get('who'), + com.get('when')) + }}

Whitespace + indentation (see above)

+{{ '%s %s' | format(' ' * com.get('level'), com.get('comment')) }} +{% if true %} {% endif %}

Is this needed?

diff --git a/osc2/cli/comment/ui.py b/osc2/cli/comment/ui.py new file mode 100644 index 0000000..1118dde --- /dev/null +++ b/osc2/cli/comment/ui.py @@ -0,0 +1,136 @@ +class PackageCommentList(CommandDescription, CommentList): + """List package comments. + + List all comments available for the package + + Example: + osc2 comment list package api://project/package + """ + cmd = 'package' + args = 'api://project/package' + func = call(CommentController.package_comment) + func_defaults = {'method': 'list', 'shell_cls': CommentShell} + opt_interactive = Option('i', 'interactive', + 'start an interactive comment shell', + action='store_true') +

We should introduce a ListOptions class for the opt_interactive option. <SNIP>

+class RequestCommentDelete(CommandDescription, Comment):

Hmm this class has nothing to do with a "request". Simply call it CommentDelete.

+ """Delete comment.

Delete a comment.

+ + Deletes comment with the given comment id. +

Deletes the comment... ?

+ Example: + osc2 comment delete api://comment

Maybe osc2 comment delete api://comment_id?

diff --git a/osc2/comments.py b/osc2/comments.py new file mode 100644 index 0000000..cd05c26 --- /dev/null +++ b/osc2/comments.py @@ -0,0 +1,72 @@ +"""Provides classes to access the comments route""" + +from osc2.core import Osc +from osc2.util.xml import OscElement, fromstring +from lxml import etree

Unused import.

+class Comments(object): + def __init__(self, *args): + self._path = self._get_path(*args) + + def _get_path(self, *args): + """Build the path for the api""" + path_base = 'comments/' + return path_base + '/'.join(args) + + def list(self, gbg1, gbg2, **kwargs):

Uhm, unused parameters. <SNIP>

+class PackageComment(Comments): + """Represents a package comment""" + def __init__(self, project, package): + super(PackageComment, self).__init__('package', project, package) + + +class ProjectComment(Comments): + """Represents a project comment""" + def __init__(self, project): + super(ProjectComment, self).__init__('project', project) + + +class RequestComment(Comments): + """Represents a request comment""" + def __init__(self, request): + super(RequestComment, self).__init__('request', request) + + +class DeleteComment(Comments): + def __init__(self): + """Represents a delete comment"""

I still don't feel really comfortable with this class hierarchy. I think we should use simple module functions as we now do in osc. comment_create(...) comment_delete(...) comments_list(...)

diff --git a/test/test_comments.py b/test/test_comments.py new file mode 100644 index 0000000..1042fef --- /dev/null +++ b/test/test_comments.py +class TestComments(OscTest):

The *_ctrl variables should be renamed (they do not represent a "controller"). <SNIP>

+ @get('http://localhost/comments/package/openSUSE%3AFactory/osc', + file='pkg_comments.xml') + def test_list_package_comments(self): + """test list package comments""" + PackageComment.SCHEMA = self.fixture_file('pkg_comment.xsd')

This has no "effect".

+ @get('http://localhost/comments/project/openSUSE%3AFactory', + file='proj_comments.xml') + def test_list_project_comments(self): + """test list package comments""" + ProjectComment.SCHEMA = self.fixture_file('proj_comment.xsd') + proj_comment_ctrl = ProjectComment('openSUSE:Factory') + projcmt = proj_comment_ctrl.list(None, None)

.list(None, None) looks fishy (see comment in the Comments class).

diff --git a/test/test_comments_fixtures/pkg_comments.xsd b/test/test_comments_fixtures/pkg_comments.xsd new file mode 100644 index 0000000..1afd75c --- /dev/null +++ b/test/test_comments_fixtures/pkg_comments.xsd

This file is not really used so far.

diff --git a/test/test_comments_fixtures/proj_comments.xsd b/test/test_comments_fixtures/proj_comments.xsd new file mode 100644 index 0000000..18b8a53 --- /dev/null +++ b/test/test_comments_fixtures/proj_comments.xsd

Unused file.

diff --git a/test/test_comments_fixtures/req_comments.xsd b/test/test_comments_fixtures/req_comments.xsd new file mode 100644 index 0000000..08e1b40 --- /dev/null +++ b/test/test_comments_fixtures/req_comments.xsd

Unused file.

[lethliel]

On 03/07/2017 03:31 PM, Marcus Hüwe wrote: <SNIP>

Some comments below. For now I skipped the whole interactive/shell code and more or less neglected the ui.py and jinja2 files.

A lot of comments. Will look at them tomorrow. Just a few first remarks. Have you documented your conventions and styleguides? I mean those that are not covered by pep8? Because I ran a pep8 and everything was fine ;)

> diff --git a/osc2/cli/comment/comment.py b/osc2/cli/comment/comment.py > new file mode 100644 > index 0000000..48ac54c > --- /dev/null > +++ b/osc2/cli/comment/comment.py > @@ -0,0 +1,199 @@ > +"""Provides controller functions for the "comment" command.""" > + > +import logging Unused? (see below)

Shouldn't we keep it anyway for future use? Or implement it when we need it in the future? <SNIP>

(Hmm maybe we should also make all class methods module functions...)

You mean no class methods at all?

> diff --git a/osc2/cli/comment/comment_list.jinja2 b/osc2/cli/comment/comment_list.jinja2 > new file mode 100644 > index 0000000..4d97221 > --- /dev/null > +++ b/osc2/cli/comment/comment_list.jinja2 > @@ -0,0 +1,12 @@ > +{% for cid, com in comments.items() %} > + {%- if (com.get('level') == 0) %} > + {{- '%s' | format( '+' * 60) }} Leading whitespace in format call. > + {%- endif %} > + {{ '%s (%s) from %s at %s:' | format( ' ' * com.get('level'), Leading whitespace in format call. > + cid, > + com.get('who'), > + com.get('when')) Indentation should be adjusted (one space less). > + }} > + {{ '%s %s' | format(' ' * com.get('level'), com.get('comment')) }} > + {% if true %} {% endif %} Is this needed? > diff --git a/osc2/cli/comment/comment_shell.jinja2 b/osc2/cli/comment/comment_shell.jinja2 > new file mode 100644 > index 0000000..68b66ad > --- /dev/null > +++ b/osc2/cli/comment/comment_shell.jinja2 > @@ -0,0 +1,10 @@ > +{% if (com.get('level') == 0) %} > +{{ '%s' | format( '+' * 60) }} Whitespace (see above) > +{% endif %} > +{{ '%s (%s) from %s at %s:' | format( ' ' * com.get('level'), > + cid, > + com.get('who'), > + com.get('when')) > + }} Whitespace + indentation (see above) > +{{ '%s %s' | format(' ' * com.get('level'), com.get('comment')) }} > +{% if true %} {% endif %} Is this needed?

Yes. Otherwise there is no newline at the end and the prompt will be on the same line as the comment. I thought indentations and whitespaces are part of jinja2 formatting.

> diff --git a/osc2/cli/comment/ui.py b/osc2/cli/comment/ui.py > new file mode 100644 > index 0000000..1118dde > --- /dev/null > +++ b/osc2/cli/comment/ui.py > @@ -0,0 +1,136 @@ > +class PackageCommentList(CommandDescription, CommentList): > + """List package comments. > + > + List all comments available for the package > + > + Example: > + osc2 comment list package api://project/package > + """ > + cmd = 'package' > + args = 'api://project/package' > + func = call(CommentController.package_comment) > + func_defaults = {'method': 'list', 'shell_cls': CommentShell} > + opt_interactive = Option('i', 'interactive', > + 'start an interactive comment shell', > + action='store_true') > + We should introduce a ListOptions class for the opt_interactive option.

So that only the list subcommand has the interactive mode?

<SNIP> > +class RequestCommentDelete(CommandDescription, Comment): Hmm this class has nothing to do with a "request". Simply call it CommentDelete. > + """Delete comment. Delete a comment. > + > + Deletes comment with the given comment id. > + Deletes the comment... ? > + Example: > + osc2 comment delete api://comment Maybe osc2 comment delete api://comment_id? > diff --git a/osc2/comments.py b/osc2/comments.py > new file mode 100644 > index 0000000..cd05c26 > --- /dev/null > +++ b/osc2/comments.py > @@ -0,0 +1,72 @@ > +"""Provides classes to access the comments route""" > + > +from osc2.core import Osc > +from osc2.util.xml import OscElement, fromstring > +from lxml import etree Unused import. > +class Comments(object): > + def __init__(self, *args): > + self._path = self._get_path(*args) > + > + def _get_path(self, *args): > + """Build the path for the api""" > + path_base = 'comments/' > + return path_base + '/'.join(args) > + > + def list(self, gbg1, gbg2, **kwargs): Uhm, unused parameters.

I prefer to call them garbage variables. ;-) Because i always give 4 args in (and if method == list comment and parent are None) result = getattr(req_comment_ctrl, method)(comment, parent, **query) But I see that *args would be better. Like: result = getattr(req_comment_ctrl, method)(comment, *args, **query) and on the other side: list(self, *args, **kwargs) create(self, *args, **kwargs) But if we bowl down the whole class concept anyway I can change it then.

<SNIP> > +class PackageComment(Comments): > + """Represents a package comment""" > + def __init__(self, project, package): > + super(PackageComment, self).__init__('package', project, package) > + > + > +class ProjectComment(Comments): > + """Represents a project comment""" > + def __init__(self, project): > + super(ProjectComment, self).__init__('project', project) > + > + > +class RequestComment(Comments): > + """Represents a request comment""" > + def __init__(self, request): > + super(RequestComment, self).__init__('request', request) > + > + > +class DeleteComment(Comments): > + def __init__(self): > + """Represents a delete comment""" I still don't feel really comfortable with this class hierarchy. I think we should use simple module functions as we now do in osc. comment_create(...) comment_delete(...) comments_list(...) > diff --git a/test/test_comments.py b/test/test_comments.py > new file mode 100644 > index 0000000..1042fef > --- /dev/null

<SNIP> I leave the tests out as long as we haven't decided on the use of classes or functions. I agree that in the case of the comments command classes have no significant advantage over module functions, but have they significant disadvantages? Just curious why you would prefer functions in this case.

[marcus-h]

On 2017-03-07 08:36:08 -0800, Marco Strigl wrote: On 03/07/2017 03:31 PM, Marcus Hüwe wrote: > Some comments below. For now I skipped the whole interactive/shell > code and more or less neglected the ui.py and jinja2 files. A lot of comments. Will look at them tomorrow. Just a few first remarks. Have you documented your conventions and styleguides? I mean those that are not covered by pep8?

Right, they are not part of pep8. The documentation format is based on pep257, but not strictly enforced... If you like I can write something up about the current "conventions".

>> diff --git a/osc2/cli/comment/comment.py b/osc2/cli/comment/comment.py >> new file mode 100644 >> index 0000000..48ac54c >> --- /dev/null >> +++ b/osc2/cli/comment/comment.py >> @@ -0,0 +1,199 @@ >> +"""Provides controller functions for the "comment" command.""" >> + >> +import logging > > Unused? (see below) > Shouldn't we keep it anyway for future use? Or implement it when we need it in the future?

It should be implemented when needed.

> (Hmm maybe we should also make all class methods module functions...) > You mean no class methods at all?

Yes, so far the CommentController simply acts as a container. A class (in this context) makes sense if there is some kind of "basic" behavior that could be (potentially) reused by other classes. For instance, see how checkout+update and review+request "interact"/reuse stuff... A "bad" example is the CommitController, because it just encapsulates some methods (I need to think about it again, why I decided to make it a class...) Heuristic (in this case): - if there's some general code that could be potentially reused, then make it a class

>> diff --git a/osc2/cli/comment/comment_shell.jinja2 b/osc2/cli/comment/comment_shell.jinja2 >> new file mode 100644 >> index 0000000..68b66ad >> --- /dev/null >> +++ b/osc2/cli/comment/comment_shell.jinja2 >> @@ -0,0 +1,10 @@ >> +{% if (com.get('level') == 0) %} >> +{{ '%s' | format( '+' * 60) }} > > Whitespace (see above) > >> +{% endif %} >> +{{ '%s (%s) from %s at %s:' | format( ' ' * com.get('level'), >> + cid, >> + com.get('who'), >> + com.get('when')) >> + }} > > Whitespace + indentation (see above) > >> +{{ '%s %s' | format(' ' * com.get('level'), com.get('comment')) }} >> +{% if true %} {% endif %} > > Is this needed? Yes. Otherwise there is no newline at the end and the prompt will be on the same line as the comment. I thought indentations and whitespaces are part of jinja2 formatting.

But can't you achieve the same with a trailing empty line without the if ... endif construct?

>> diff --git a/osc2/cli/comment/ui.py b/osc2/cli/comment/ui.py >> new file mode 100644 >> index 0000000..1118dde >> --- /dev/null >> +++ b/osc2/cli/comment/ui.py >> @@ -0,0 +1,136 @@ > >> +class PackageCommentList(CommandDescription, CommentList): >> + """List package comments. >> + >> + List all comments available for the package >> + >> + Example: >> + osc2 comment list package api://project/package >> + """ >> + cmd = 'package' >> + args = 'api://project/package' >> + func = call(CommentController.package_comment) >> + func_defaults = {'method': 'list', 'shell_cls': CommentShell} >> + opt_interactive = Option('i', 'interactive', >> + 'start an interactive comment shell', >> + action='store_true') >> + > We should introduce a ListOptions class for the opt_interactive > option. So that only the list subcommand has the interactive mode?

Yes (that's also how it is currently implemented). The idea is to define the opt_interactive class variable only once instead of 3 times (cf. the CreateOptions class).

>> +class Comments(object): >> + def __init__(self, *args): >> + self._path = self._get_path(*args) >> + >> + def _get_path(self, *args): >> + """Build the path for the api""" >> + path_base = 'comments/' >> + return path_base + '/'.join(args) >> + >> + def list(self, gbg1, gbg2, **kwargs): > > Uhm, unused parameters. I prefer to call them garbage variables. ;-)

Ah now I see... Short answer: something like this cannot get merged - no way!:) Rationale: everything that is located in osc2/* (and not in osc2/cli/*) is part of a public API that is supposed to be used by others. Having a method that takes two required parameters, which are never used is a bad idea (nobody wants to use such an interface...).

Because i always give 4 args in (and if method == list comment and parent are None) result = getattr(req_comment_ctrl, method)(comment, parent, **query) But I see that *args would be better. Like: result = getattr(req_comment_ctrl, method)(comment, *args, **query) and on the other side: list(self, *args, **kwargs) create(self, *args, **kwargs) But if we bowl down the whole class concept anyway I can change it then.

Nah... I'm sure we will find a nice solution while keeping the classes:)

> <SNIP> > >> +class PackageComment(Comments): >> + """Represents a package comment""" >> + def __init__(self, project, package): >> + super(PackageComment, self).__init__('package', project, package) >> + >> + >> +class ProjectComment(Comments): >> + """Represents a project comment""" >> + def __init__(self, project): >> + super(ProjectComment, self).__init__('project', project) >> + >> + >> +class RequestComment(Comments): >> + """Represents a request comment""" >> + def __init__(self, request): >> + super(RequestComment, self).__init__('request', request) >> + >> + >> +class DeleteComment(Comments): >> + def __init__(self): >> + """Represents a delete comment""" > > I still don't feel really comfortable with this class hierarchy. > I think we should use simple module functions as we now do in osc. > > comment_create(...) > comment_delete(...) > comments_list(...) > >> diff --git a/test/test_comments.py b/test/test_comments.py >> new file mode 100644 >> index 0000000..1042fef >> --- /dev/null <SNIP> I leave the tests out as long as we haven't decided on the use of classes or functions. I agree that in the case of the comments command classes have no significant advantage over module functions, but have they significant disadvantages?

Having a class could be handy for the "comment shell": we can just pass the concrete object to the shell and the shell can simply call ".comment_create()", ".comments_list()" etc. without knowing the "kind" and "args".

Just curious why you would prefer functions in this case.

My main concern is that the names are misleading (and also the class' descriptions). First, when reading "class PackageComment" I would expect/guess that this class represents a comment that is part of a package. Reading the docstring affirms this guess/expectation. Reading the code reveals that my guess/expectation is plain wrong. Second, for such a simple inheritance hierarchy I would have expected some kind of an IS-A relationship. That is, "PackageComment" is a "Comments", which sounds "strange". I would propose the following: class Commentable(object): def __init__(self, kind, *args): ... def comment_create(self, comment (or comment_text), parent, **kwargs): ... def comments_list(self, **kwargs): ... # 2 options: # - implement the delete logic here # - implement the delete logic in a module function and delegate # to the module function @classmethod def comment_delete(cls, comment_id, **kwargs): return comment_delete(comment_id, **kwargs) class Project(Commentable): ... class Package(Commentable): ... class Request(Commentable): ... (with the appropriate constructors) # only needed if the we chose the second option... def comment_delete(comment_id, **kwargs): ... What do you think?