2060_acceptance_review #84
Conversation
…0_acceptance_review
… avoid re-mapping the word builtins in setup.
Also cleaned up some blank space issues.
…, not sure how this was ever here
In the previous commit the chance that the variable 'cutout' might be undefined, because this isn't a cutout call, was overlooked. Just catch this possibility and ignore the cutout building step. Also, the new path matching pattern is a bit more generic but also weaker if the path contains characters that are not normal so the path needed to be normalized first.
The astropy template uses the definition of the _PACKAGE_SETUP_ variable to help the test system to know if a module is being called during the setup.py process. This is no longer used by vostools
This changes are all to bring code in line with PEP 8 format and add documentation. Also added some TODOs where code modifications would be good for long term maintenance.
This also involved re-arranging how the parameters are set as we're migrating to argparser.
…sage reporting The open command in some of the methods were being passed unicode strings but expected str so I cast to str. Copy errors now reported more cleanly to the user.
This is a major rework of how the help messages are handled and how arguments are parsed. Now rely on argparse to get required arguments and do counting of these arguments.
This commit also did minor PEP8 cleanup.
There was a problem hiding this comment.
I don't like relative imports. They come handy when code within the package is moved around. However, the can be a big pain if you for example want to do a quick test in the file by throwing in a "main". In that case the relative imports stop working unless you invoke the file as a module or define a package var. IMO, at least in our case, the drawbacks of using relative imports outweigh the benefits and I've decided to avoid using them.
vos/vos/commands/vcat.py
Outdated
|
|
||
| (opt, args) = parser.parse_args() | ||
| parser.process_informational_options() | ||
| parser = CommonParser(description=description) |
There was a problem hiding this comment.
description is not defined and it's currently failing.
Isn't it simpler to make the description the doctring of the vcat function and pass it to CommonParser as CommonParser(description=vcat.doc)? (double underscore around doc)
vos/vos/commands/vcp.py
Outdated
| where RA, DEC and RAD are all given in degrees | ||
|
|
||
| If no X509 certificate given on commnad line then default location will be used. | ||
| """ |
There was a problem hiding this comment.
What is the default location?
There was a problem hiding this comment.
Default location should be spelled out as ${HOME}/.ssl/cadcproxy.pem
| "(?P<ra>[\-\+]?\d*(\.\d*)?)," | ||
| "(?P<dec>[\-\+]?\d*(\.\d*)?)," | ||
| "(?P<ra>[\-+]?\d*(\.\d*)?)," | ||
| "(?P<dec>[\-+]?\d*(\.\d*)?)," |
There was a problem hiding this comment.
Backslash removed unintentionally? They seem to be needed.
There was a problem hiding this comment.
The backslashes are not needed here. At least that's what my testing shows.
vos/vos/commands/vls.py
Outdated
| else: | ||
| length = str(int(size)) | ||
| return "%12s " % length | ||
| def get_terminal_size(): |
There was a problem hiding this comment.
hmm. I think get_terminal_size was in use at one time. Perhaps not any more. The idea was to do column display of file names. I think I couldn't get this working the way I wanted and so never used in production.
|
|
||
| class CommonParser(argparse.ArgumentParser): |
There was a problem hiding this comment.
Too bad we could not come to a common approach with the rest of the CADC apps. In essence, this does the same thing (although differently) with the code in cadcutils.
There was a problem hiding this comment.
One parser to rule them all!
|
I've pushed changes to address the issues you raised and fix the DESCRIPTION variable. However I still use imports with implicit path structure. We should discuss this and figure out if there is best practise somewhere. |
|
https://stackoverflow.com/questions/4209641/absolute-vs-explicit-relative-import-of-python-module
On Jul 28, 2017, at 11:30 AM, JJ Kavelaars <notifications@github.com<mailto:notifications@github.com>> wrote:
I've pushed changes to address the issues you raised and fix the DESCRIPTION variable. However I still use imports with implicit path structure. We should discuss this and figure out if there is best practise somewhere.
—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub<#84 (comment)>, or mute the thread<https://github.com/notifications/unsubscribe-auth/AF04LtDeJmqS8xFeWigFiB7aXE1MGxInks5sSijCgaJpZM4OkS5T>.
|
Improved documentation. Passes most integration tests.