RPPT-001 | Rewrite listener, fix rp_logger. Make code more pythonic. #35

lebovski · 2017-11-21T07:46:20Z

Rewritten listener class for support pytest fixtures
Improved RPLogger class for correct write test info
Make code more pythonic
The listener code is in a separate file
The logger code is in a separate file
Used html library instead of cgi

krasoffski · 2017-11-21T07:58:23Z

@lebovski, many thanks!
I will take a look at you PR.

krasoffski

It is a brief review for obvious changes. I'll continue after work.

krasoffski · 2017-11-21T07:59:45Z

pytest_reportportal/__init__.py

-            loglevel=self._loglevel_map[level],
-            attachment=record.__dict__.get("attachment", None),
-        )
+from .rp_logging import RPLogger, RPLogHandler


To avoid warn about not used imports, please add

__all__ = [RPLogger, RPLogHandler]

krasoffski · 2017-11-21T08:02:33Z

pytest_reportportal/plugin.py

    group = parser.getgroup("reporting")
    group.addoption(
-        "--rp-launch",
+        '--rp-launch',


What is the reason to change quotes here and keep double quotes in other places?

krasoffski · 2017-11-21T08:05:19Z

pytest_reportportal/service.py

            logging.debug(
-                msg="ReportPortal - Init service: "
-                    "endpoint={0}, project={1}, uuid={2}".
+                msg='ReportPortal - Init service: endpoint={0}, project={1}, uuid={2}'.


Please limit line length up to 79 chars (inclusive).

krasoffski · 2017-11-21T08:09:11Z

pytest_reportportal/listener.py

@@ -0,0 +1,45 @@
+import html


Does html module exist in the Python2.7? Also this module is not a dependency of pytest. Than is why solution with cgi was chosen.

You are right. Fixed.

krasoffski · 2017-11-21T08:11:50Z

pytest_reportportal/rp_logging.py

+    def __init__(self, name, level=0):
+        super(RPLogger, self).__init__(name, level=level)
+
+    def _log(self, level, msg, args, exc_info=None, extra=None, stack_info=False, attachment=None):


Length up to 79 chars.

krasoffski · 2017-11-21T08:12:20Z

pytest_reportportal/rp_logging.py

+        if exc_info and not isinstance(exc_info, tuple):
+            exc_info = sys.exc_info()
+
+        record = self.makeRecord(self.name, level, fn, lno, msg, args, exc_info, func, extra, sinfo)


79 chars here, please check other places for this.

krasoffski · 2017-11-21T08:20:23Z

.gitignore

@@ -0,0 +1,2 @@
+# Created by .ignore support plugin (hsz.mobi)


FYI, it is a good practice to split a list of changes using commits. This simplifies review and keeps attention on important topics.
E.g.
1 commit: update quotes
2 commit: add support of fixtures
3 commit: something else

During a review you will see squashed changes, but if you want to review a particular topic you can do this with approach above but with big commit like yours it is not possible (in case of renamed files it is quite difficult)

I completely agree. Thanks for the advice.

I will follow this advice in next time.

krasoffski · 2017-11-21T19:33:13Z

pytest_reportportal/rp_logging.py

+        super(RPLogHandler, self).__init__(level)
+
+        # Map loglevel codes from `logging` module to ReportPortal text names:
+        self._loglevel_map = {


What is the reason to move _loglevel_map to __init__?

krasoffski

Added some comments after review.

krasoffski · 2017-11-21T19:45:29Z

setup.py

-tar_url = "https://github.com/reportportal/agent-python-pytest/tarball/1.0.0"
+version = '1.1.0'
+tar_url = 'https://github.com/reportportal/agent-python-pytest/tarball/1.0.0'
+


I think it is 1.0.1 because there is no major features, only defect fixes.
Also, do not forget to update tar_url path for tarball.

krasoffski · 2017-11-21T19:48:03Z

pytest_reportportal/plugin.py

-        log_batch_size=int(session.config.getini("rp_log_batch_size")),
-        ignore_errors=bool(session.config.getini("rp_ignore_errors")),
-        ignored_tags=session.config.getini("rp_ignore_tags"),
+        project=session.config.getini('rp_project'),


What is the main reason to use ' single quote instead of " double quote?

You are using single and double quotes in code, for uniformity of the written code it is better to use only single or double, i prefer single.

krasoffski · 2017-11-22T07:29:15Z

Merged, but would be great to test the latest version.

RPPT-001 | Rewrite listener, fix rp_logger. Make code more pythonic.

fb80c13

krasoffski suggested changes Nov 21, 2017

View reviewed changes

alexandr added 4 commits November 21, 2017 08:39

RPPT-001 | Add __all__ to __init__

3cf0275

RPPT-001 | Use 79 char length

f841fe5

RPPT-001 | Fix quotes

f75b0ae

RPPT-001 | Use cgi instead html for support python 2.7

5a7bd63

krasoffski reviewed Nov 21, 2017

View reviewed changes

krasoffski suggested changes Nov 21, 2017

View reviewed changes

alexandr added 2 commits November 22, 2017 06:51

RPPT-001 | Move loglevel map from __init__

3adfc5b

RPPT-001 | version fix

0d5f82c

krasoffski approved these changes Nov 22, 2017

View reviewed changes

krasoffski merged commit b64f195 into reportportal:master Nov 22, 2017

		@@ -0,0 +1,2 @@
		# Created by .ignore support plugin (hsz.mobi)

RPPT-001 | Rewrite listener, fix rp_logger. Make code more pythonic. #35

RPPT-001 | Rewrite listener, fix rp_logger. Make code more pythonic. #35

Uh oh!

Conversation

lebovski commented Nov 21, 2017

Uh oh!

krasoffski commented Nov 21, 2017

Uh oh!

krasoffski left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

krasoffski Nov 21, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

krasoffski Nov 21, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

krasoffski Nov 21, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

krasoffski left a comment

Choose a reason for hiding this comment

Uh oh!

krasoffski Nov 21, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

krasoffski commented Nov 22, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

krasoffski left a comment •

edited

Loading

krasoffski Nov 21, 2017 •

edited

Loading

krasoffski Nov 21, 2017 •

edited

Loading

krasoffski Nov 21, 2017 •

edited

Loading

krasoffski Nov 21, 2017 •

edited

Loading