Refactors amclient.py #46

ross-spencer · 2018-02-20T00:22:16Z

WIP. Hi @jrwdunham @jraddaoui can I request your review of this work? I wanted to separate the logic of the various components of amclient.py to make it clearer to extend in future. I want to base a script for a client of this work in the coming days. The summary is below.

Before this can be merged, I am having a problem with ModuleImportError in the unit tests. I have tried my best to solve this today but to no avail. The scripts work independently as executable scripts, but it seems there maybe an issue testing that at the package level, and it seems like this has changed in Python 3.6. The tests pass in 2.7.

Your view on the refactor, and the test issue would be greatly appreciated. I'll try and take a look later.

Creates new logging configuration script to reduce redundancy
Separates responsibility of amclient, defaults, and cli args
Cleans up argparse output using metavar
Adds Conformity to PEP8 to amclient.py and transfers.py
Returns user friendly string if arg evaluation fails in main
Improved error return from JSON request function in amclient.py
Adds .db and .lck files to .gitignore
Works at module and executable level
E402 flake8 warning ignored in Tox to enable module/exe to work
Captures connection errors in transfers.py better
Initial changes provides scope for future refactoring as required
Addresses Problem: The Automation Tools AMClient script could be easier to jump into and add functionality #47

jraddaoui · 2018-02-20T17:07:15Z

Hi @ross-spencer, the imports issue can be fixed by using from . import defaults instead of import defaults and the same for other relative imports you added. This may be related:

https://docs.pytest.org/en/latest/goodpractices.html#tests-as-part-of-application-code

jraddaoui · 2018-02-20T17:07:31Z

Sorry, wrong button.

ross-spencer · 2018-02-20T20:05:08Z

@jraddaoui @jrwdunham thanks for your advice. It was a pain to fix. Joel made another PR for me to look at and I used the change from that plus imported the module to the path which you'll see in the code as sys.path.append('../'). I was finding three or four different ways of importing these files as modules/scripts etc. Getting it to work standalone and then watching the tests fail, and vice versa. I think this is the best I've found to allow us to pull apart some of the code to make individual scripts a little simpler, and have everything work as expected. I had to disable a flake8 warning around imports:

E402 # Module level imports not at top of file

But I hope that's okay.

I'm going to have a look in more detail at our client's requirements and maybe use this base as a premise. Let me know what changes I can make as required. Hope it's mostly okay though.

jrwdunham

Hey Ross, overall I like the refactor. I have some comments I'd like you to address.

As a general point, I think it would be preferable if you could open an issue that clearly explains the problem you are trying to solve using the prefix "Problem: ..." and then document your proposed changes in the description of that issue and name your dev branch dev/issue-<ISSUE_NO>-short-description.

On another note, I'm also wondering if it might not be useful to add setuptools functionality to autotools along with this PR, including perhaps adding scripts or entry_points kwargs to setup so that installing autotools will add system-wide executables, cf. http://python-packaging.readthedocs.io/en/latest/command-line-scripts.html — just a thought.

jrwdunham · 2018-02-20T23:05:12Z

tests/test_amclient.py

@@ -24,6 +24,7 @@


 class TmpDir:
+


Nitpick: why add the extra space here?

This is showing up as a flake8 error locally.

jrwdunham · 2018-02-20T23:17:06Z

transfers/amclient.py

+            args.subcommand.replace('-', '_')))
+        if func:
+            res = func()
+            if type(res) is dict:


Nitpick: you should use isinstance(res, dict) here instead of type(res) is dict.

jrwdunham · 2018-02-20T23:27:10Z

transfers/amclient.py

 from __future__ import print_function, unicode_literals

-import argparse
+import os
+


Nitpick: os and all the imports in the next "paragraph" are all part of the standard library. The pattern we've been following it to create a block of standard library imports, then a block of third party imports, then a block of local imports. I think os should be placed before pprint.

I hadn't appreciated this, but works for me.

jrwdunham · 2018-02-20T23:57:38Z

transfers/amclient.py

    try:
-        getattr(am_client, 'print_{0}'.format(args.subcommand.replace('-', '_')))
+        func = getattr(am_client, '{0}'.format(


Why did you remove the 'print_ here? I believe that as you have it now, the __getattr__ method will not be called and as a result the stdout method will not be called and the self.output_mode attribute will have no effect. (Correct me if I'm wrong.) I think the original line 591 should be restored and your lines 395-406 should be moved to AMClient.__getattr__.

If you do have a good reason to remove the 'print_, then you should remove the call to .format since it would be unnecessary.

The primary reason was that in being verbose here we have an opportunity to write a user friendly output when there is an error condition, e.g. connecting on the wrong port:

ross-spencer@artefactual:~/git/artefactual/automation-tools/transfers:$ python amclient.py completed-transfers test {u'message': u'Fetched completed transfers successfully.', u'results': []} ross-spencer@artefactual:~/git/artefactual/automation-tools/transfers:$ python amclient.py completed-transfers test Error connecting to the server, check amclient log

Where before, relying on the previous call and error handling we'd get a result of Null at the terminal, or, given a string returned from the calling function, would still have its formatting in place, e.g. as a return, "Error connecting to the server, check amclient log\n"

You're right about .format.

If I remove that, it does maintain the expected behaviour which I think might be enough to keep this change as-is, but what do you think?

jrwdunham · 2018-02-20T23:59:12Z

transfers/loggingconfig.py

+        level = max(level, -1)  # No smaller than -1
+        level = min(level, 2)  # No larger than 2
+        return log_levels[level]
+    else:


Nitpick: unnecessary else after return

jrwdunham · 2018-02-21T00:16:06Z

transfers/transfer.py

    except:
-        LOGGER.info('This script is already running. To override this behaviour and start a new run, remove %s', pid_file)
+        LOGGER.info(('This script is already running. To override this '
+                     'behaviour and start a new run, remove %s'), pid_file)


You don't need the extra set of parens here.

jrwdunham · 2018-02-21T00:16:58Z

transfers/transfer.py

-            str(microservice != current_unit.microservice),  # String True or False if this is the first time at this wait point
+            str(microservice != current_unit.microservice),
+            # String True or False if this is the first time at this wait
+            # point


Nitpick: I think the descriptive comment should go above the thing it describes.

jrwdunham · 2018-02-21T00:28:41Z

transfers/amclient.py

-                '--' + opt.name, metavar=opt.metavar, help=opt.help,
-                default=opt.default, type=opt.type)
-    return parser
+import urllib3


I think that you need to add urllib3 to the requirements/base.txt file. urllib3 became a dependency of requests but only as of a certain version (I don't know exactly which). Because our requests requirement is vague, it's possible to have requests installed but not urllib3. The other option would be to pin down a more specific (higher) requests version in our requirements.

jrwdunham · 2018-02-21T00:34:31Z

transfers/transfer.py

+    parser.add_argument(
+        '--log-level', choices=['ERROR', 'WARNING', 'INFO', 'DEBUG'],
+        default=None, help='Set the debugging output level. This will \
+        override -q and -v')


MegaNitpick: "The preferred way of wrapping long lines is by using Python's implied line continuation inside parentheses, brackets and braces. Long lines can be broken over multiple lines by wrapping expressions in parentheses. These should be used in preference to using a backslash for line continuation." From https://www.python.org/dev/peps/pep-0008/.

In the case of breaking strings across lines, I think the parenthesis with implicit concatenation strategy is better because you don't end up having strange sequences of whitespace in the middle of your strings. In this context it doesn't matter because argparse is getting rid of them for you, but in general, it's one reason beyond an appeal to authority for this preference.

I do agree. I should have fixed that now with the next commit when it arrives.

jrwdunham · 2018-02-21T00:37:59Z

transfers/transfer.py

+    log_level = loggingconfig.set_log_level(
+        args.log_level, args.quiet, args.verbose)
+
+    global CONFIG_FILE


I think Python's global keyword should be avoided in general. I would like to avoid adding more globals to the codebase. Why not just add a config_file param to get_setting?

Should be good now.

* Creates new logging configuration script to reduce redundancy * Separates responsibility of amclient, defaults, and cli args * Cleans up argparse output using metavar * Adds Conformity to PEP8 to amclient.py and transfers.py * Returns user friendly string via getattr if function call fails in main * Improved error return from JSON request function in amclient.py * Adds .db and .lck files to .gitignore * Works at module and executable level * E402 flake8 warning ignored in Tox to enable module/exe to work * Captures connection errors in transfers.py better * Initial changes provides scope for future refactoring as required

ross-spencer · 2018-02-21T05:43:25Z

Good stuff. I have committed an initial tranche of changes tonight. Will look at adding setuptools functionality in the morning. I think it sounds like a great change for folks who might want to work with this script.

Edit: The tests are failing. The config file change to transfers.py will take a little more effort.

ross-spencer added the work-in-progress label Feb 20, 2018

ross-spencer requested review from jraddaoui and jrwdunham February 20, 2018 00:22

qubot force-pushed the dev/refactor-am-client-responsibilities branch from 77c227b to 1a5329d Compare February 20, 2018 16:29

jraddaoui closed this Feb 20, 2018

jraddaoui reopened this Feb 20, 2018

qubot force-pushed the dev/refactor-am-client-responsibilities branch from 1a5329d to 70e0e8e Compare February 20, 2018 19:55

jrwdunham suggested changes Feb 21, 2018

View reviewed changes

qubot force-pushed the dev/refactor-am-client-responsibilities branch from 70e0e8e to 5ad2b77 Compare February 21, 2018 05:41

sevein mentioned this pull request Feb 21, 2018

Problem: The Automation Tools AMClient script could be easier to jump into and add functionality #47

Closed

qubot closed this Feb 21, 2018

qubot deleted the dev/refactor-am-client-responsibilities branch February 21, 2018 20:26

ross-spencer mentioned this pull request Feb 21, 2018

Refactors amclient.py #48

Merged

ross-spencer mentioned this pull request May 17, 2018

Problem: Make the Automation Tools take advantage of Python Setup Tools #73

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactors amclient.py #46

Refactors amclient.py #46

ross-spencer commented Feb 20, 2018 •

edited

Loading

jraddaoui commented Feb 20, 2018

jraddaoui commented Feb 20, 2018

ross-spencer commented Feb 20, 2018 •

edited

Loading

jrwdunham left a comment •

edited

Loading

jrwdunham Feb 20, 2018

ross-spencer Feb 21, 2018

jrwdunham Feb 20, 2018

ross-spencer Feb 21, 2018

jrwdunham Feb 20, 2018

ross-spencer Feb 21, 2018

jrwdunham Feb 20, 2018

jrwdunham Feb 21, 2018

ross-spencer Feb 21, 2018 •

edited

Loading

jrwdunham Feb 20, 2018

jrwdunham Feb 21, 2018

ross-spencer Feb 21, 2018

jrwdunham Feb 21, 2018

jrwdunham Feb 21, 2018

jrwdunham Feb 21, 2018

ross-spencer Feb 21, 2018

jrwdunham Feb 21, 2018

ross-spencer Feb 21, 2018

ross-spencer commented Feb 21, 2018 •

edited

Loading

Refactors amclient.py #46

Refactors amclient.py #46

Conversation

ross-spencer commented Feb 20, 2018 • edited Loading

jraddaoui commented Feb 20, 2018

jraddaoui commented Feb 20, 2018

ross-spencer commented Feb 20, 2018 • edited Loading

jrwdunham left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ross-spencer Feb 21, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ross-spencer commented Feb 21, 2018 • edited Loading

ross-spencer commented Feb 20, 2018 •

edited

Loading

ross-spencer commented Feb 20, 2018 •

edited

Loading

jrwdunham left a comment •

edited

Loading

ross-spencer Feb 21, 2018 •

edited

Loading

ross-spencer commented Feb 21, 2018 •

edited

Loading