New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Documentation for command line interfaces #1090

Merged
merged 9 commits into from Jun 20, 2017

Conversation

Projects
None yet
9 participants
@ghoshbishakh
Member

ghoshbishakh commented Jun 27, 2016

No description provided.

@coveralls

This comment has been minimized.

coveralls commented Jun 27, 2016

Coverage Status

Coverage remained the same at 82.88% when pulling e57dbb0 on ghoshbishakh:docforcmd into 220d883 on nipy:master.

@codecov-io

This comment has been minimized.

codecov-io commented Jun 27, 2016

Codecov Report

Merging #1090 into master will increase coverage by <.01%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1090      +/-   ##
==========================================
+ Coverage   87.04%   87.05%   +<.01%     
==========================================
  Files         226      226              
  Lines       28262    28262              
  Branches     3026     3026              
==========================================
+ Hits        24602    24603       +1     
  Misses       2976     2976              
+ Partials      684      683       -1
Impacted Files Coverage Δ
dipy/core/graph.py 75% <0%> (+1.19%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 599af3e...12156a2. Read the comment docs.

from distutils.version import LooseVersion as V


def sh3(cmd):

This comment has been minimized.

@MarcCote

MarcCote Jun 27, 2016

Contributor

What does sh3 stands for?

This comment has been minimized.

@ghoshbishakh

ghoshbishakh Jun 27, 2016

Member

I was looking at how other projects generate documentation. This is taken from a script which I really liked. I did not bother to change the name actually. 😋
I can change it if you suggest doing so.
https://github.com/scikit-image/scikit-image/blob/master/doc/gh-pages.py

This comment has been minimized.

@MarcCote

MarcCote Jun 27, 2016

Contributor

Oh, I see. It's the third version of a function that runs a command in a shell. We can keep that name and add the link you provided in the Notes section of the docstring.

This comment has been minimized.

@arokem

arokem Mar 23, 2017

Member

Please add this link to the docstring.

And the scikit image license.

This comment has been minimized.

@arokem

arokem May 12, 2017

Member

Please don't forget to add the scikit-image license.

This comment has been minimized.

@ghoshbishakh

ghoshbishakh May 12, 2017

Member

Should I include it in this docstring? @arokem

@jchoude

This comment has been minimized.

Contributor

jchoude commented Jun 27, 2016

Next time, please add at least a quick description of the goal of the PR. For @Garyfallidis and I, it's easy to guess since it's part of your project, but for other reviewers, it might not be as easy.

@@ -37,6 +37,8 @@ api-clean:
api:
@mkdir -p reference
$(PYTHON) tools/build_modref_templates.py dipy reference
@mkdir -p reference_cmd
$(PYTHON) tools/docgen_cmd.py dipy reference_cmd

This comment has been minimized.

@jchoude

jchoude Jun 27, 2016

Contributor

No strong opinion on this, but do others think this might be better moved in a separate section, called something like command-line-api.

"""
Script to generate documentation for command line utilities
"""
import sys

This comment has been minimized.

@jchoude

jchoude Jun 27, 2016

Contributor

Import ordering.



def abort(error):
print('*WARNING* API documentation not generated: %s' % error)

This comment has been minimized.

@jchoude

jchoude Jun 27, 2016

Contributor

Command line API ....

try:
__import__(package)
except ImportError as e:
abort("Can not import " + package)

This comment has been minimized.

@jchoude

jchoude Jun 27, 2016

Contributor

Can not -> Cannot

@jchoude

This comment has been minimized.

Contributor

jchoude commented Jun 27, 2016

Also, general question: I'm trying to build the API documentation, to test this PR. However, I work in a virtualenv. When it tries to call the bin/* scripts, there only is the dipy_median_otsu that works. All other 4 scripts have a bad interpreter error.

This happens because of the shebang line, which is

#!python

in the failing scripts, while it is

#! /usr/bin/env python

Do we have a standard for this. StackOverflow seems to prefer the second version, since it honors the environment (and hence, why it works in my venv), but in some cases people advise against it.

Thoughts?

@ghoshbishakh

This comment has been minimized.

Member

ghoshbishakh commented Jun 29, 2016

It is confusing. For me only dipy_median_otsu does not work (while not using virtual environment).

@coveralls

This comment has been minimized.

coveralls commented Jun 29, 2016

Coverage Status

Coverage increased (+0.03%) to 82.908% when pulling e9241c2 on ghoshbishakh:docforcmd into 220d883 on nipy:master.

@ghoshbishakh ghoshbishakh changed the title from Documentation for command line interfaces to WIP Documentation for command line interfaces Jul 11, 2016

@Garyfallidis

This comment has been minimized.

Member

Garyfallidis commented Jul 12, 2016

As far as I know the #!python is the correct shebang line. Is that correct @matthew-brett ?

@matthew-brett

This comment has been minimized.

Member

matthew-brett commented Jul 12, 2016

Yes, that's correct. With the "!python" shebang line, the scripts should install correctly via pip, at least.

@ghoshbishakh

This comment has been minimized.

Member

ghoshbishakh commented Jul 13, 2016

@Garyfallidis @matthew-brett yes using #!python , the script becomes executable as a command. But in PR #1087 there is only one workflow (for now) and that uses #! /usr/bin/env python
So does that need to be changed?

@jchoude

This comment has been minimized.

Contributor

jchoude commented Jul 19, 2016

Ok, I see that you say that's the correct way. I guess it works ok if I do a make install. But, if I just checkout a branch, set it in my python path, and work in a venv at the same time, the #!python shebang always gives me an interpreter error.

Calling it with using the python script.py instead of ./script.py works.

I don't know if we want to support my use case.

@jchoude

This comment has been minimized.

Contributor

jchoude commented Jul 19, 2016

Also, do we want to be able to call the doc/tools/docgen_cmd.py script directly, or only through the makefile? If we want to be able to call it directly, might be useful to have a help message about the expected arguments, to help users.

@ghoshbishakh

This comment has been minimized.

Member

ghoshbishakh commented Jul 20, 2016

@jchoude It will be called from the makefile. Just like build_examples.py works. But I will give a help text for arguments if any.

@MarcCote

This comment has been minimized.

Contributor

MarcCote commented Jul 22, 2016

@jchoude: I had a lot of problems when messing with my PYTHONPATH. Now, what I do is pip install -e . which simply adds a link in the site-packages/ to your project folder. It works fine within virtualenv too.

@ghoshbishakh ghoshbishakh force-pushed the ghoshbishakh:docforcmd branch from e9241c2 to ac645df Aug 13, 2016

@coveralls

This comment has been minimized.

coveralls commented Aug 13, 2016

Coverage Status

Coverage remained the same at 83.025% when pulling ac645df on ghoshbishakh:docforcmd into 31bc6fe on nipy:master.

for f in listdir(bin_folder):
if f.startswith("dipy_"):
try:
help_string, err = sh3("%s -h" % (f,))

This comment has been minimized.

@arokem

arokem Mar 23, 2017

Member

This is where we need to suppress warnings

@ghoshbishakh

This comment has been minimized.

Member

ghoshbishakh commented May 8, 2017

@arokem
The warning is marked as:
warnings.simplefilter('always', DeprecationWarning)
Is it possible to ignore that warning?

https://github.com/nipy/dipy/blob/master/dipy/denoise/nlmeans.py#L8

@arokem

This comment has been minimized.

Member

arokem commented May 8, 2017

@ghoshbishakh

This comment has been minimized.

Member

ghoshbishakh commented May 8, 2017

@arokem No what i wanted to say is that should I change the nlmeans file to change the warning filter?
As with warnings.simplefilter("always"), I could not find any way to ignore the warning through command line. However after removing the filter line we should be able to ignore it with python -W ignore filename.py

@arokem

This comment has been minimized.

Member

arokem commented May 10, 2017

No - don't change the warning in nlmeans! It's there for a reason -- that function is deprecated and we will want to eventually remove it altogether. So users that use this function should be warned that they should start changing their code to use the other implementation of this function.

@ghoshbishakh

This comment has been minimized.

Member

ghoshbishakh commented May 11, 2017

So DeprecationWarning is ignored by default.

That is why warnings.simplefilter('always', DeprecationWarning) is used.

But this makes it difficult to ignore it this way.

With python -W file.py I get the output:

Options and arguments (and corresponding environment variables):
-b     : issue warnings about str(bytes_instance), str(bytearray_instance)
         and comparing bytes/bytearray with str. (-bb: issue errors)
-B     : don't write .py[co] files on import; also PYTHONDONTWRITEBYTECODE=x
-c cmd : program passed in as string (terminates option list)
-d     : debug output from parser; also PYTHONDEBUG=x
-E     : ignore PYTHON* environment variables (such as PYTHONPATH)
-h     : print this help message and exit (also --help)
-i     : inspect interactively after running script; forces a prompt even
         if stdin does not appear to be a terminal; also PYTHONINSPECT=x
-I     : isolate Python from the user's environment (implies -E and -s)
-m mod : run library module as a script (terminates option list)
-O     : optimize generated bytecode slightly; also PYTHONOPTIMIZE=x
-OO    : remove doc-strings in addition to the -O optimizations
-q     : don't print version and copyright messages on interactive startup
-s     : don't add user site directory to sys.path; also PYTHONNOUSERSITE
-S     : don't imply 'import site' on initialization
-u     : unbuffered binary stdout and stderr, stdin always buffered;
         also PYTHONUNBUFFERED=x
         see man page for details on internal buffering relating to '-u'
-v     : verbose (trace import statements); also PYTHONVERBOSE=x
         can be supplied multiple times to increase verbosity
-V     : print the Python version number and exit (also --version)
         when given twice, print more information about the build
-W arg : warning control; arg is action:message:category:module:lineno
         also PYTHONWARNINGS=arg
-x     : skip first line of source, allowing use of non-Unix forms of #!cmd
-X opt : set implementation-specific option
file   : program read from script file
-      : program read from stdin (default; interactive mode if a tty)
arg ...: arguments passed to program in sys.argv[1:]

Other environment variables:
PYTHONSTARTUP: file executed on interactive startup (no default)
PYTHONPATH   : ':'-separated list of directories prefixed to the
               default module search path.  The result is sys.path.
PYTHONHOME   : alternate <prefix> directory (or <prefix>:<exec_prefix>).
               The default module search path uses <prefix>/pythonX.X.
PYTHONCASEOK : ignore case in 'import' statements (Windows).
PYTHONIOENCODING: Encoding[:errors] used for stdin/stdout/stderr.
PYTHONFAULTHANDLER: dump the Python traceback on fatal errors.
PYTHONHASHSEED: if this variable is set to 'random', a random value is used
   to seed the hashes of str, bytes and datetime objects.  It can also be
   set to an integer in the range [0,4294967295] to get hash values with a
   predictable seed.
PYTHONMALLOC: set the Python memory allocators and/or install debug hooks
   on Python memory allocators. Use PYTHONMALLOC=debug to install debug
   hooks.

So this adds the unnecessary section below.

@ghoshbishakh ghoshbishakh force-pushed the ghoshbishakh:docforcmd branch from ac645df to b5f849f May 11, 2017

@coveralls

This comment has been minimized.

coveralls commented May 11, 2017

Coverage Status

Coverage remained the same at 88.591% when pulling b5f849f on ghoshbishakh:docforcmd into 7d98cae on nipy:master.

@ghoshbishakh ghoshbishakh changed the title from WIP Documentation for command line interfaces to Documentation for command line interfaces May 12, 2017

@ghoshbishakh

This comment has been minimized.

Member

ghoshbishakh commented May 12, 2017

Warnings have been removed

@coveralls

This comment has been minimized.

coveralls commented May 12, 2017

Coverage Status

Coverage remained the same at 88.591% when pulling 2ecd5c6 on ghoshbishakh:docforcmd into 7d98cae on nipy:master.

@coveralls

This comment has been minimized.

coveralls commented May 12, 2017

Coverage Status

Coverage remained the same at 88.591% when pulling 2ecd5c6 on ghoshbishakh:docforcmd into 7d98cae on nipy:master.

@coveralls

This comment has been minimized.

coveralls commented May 12, 2017

Coverage Status

Coverage remained the same at 88.591% when pulling 2ecd5c6 on ghoshbishakh:docforcmd into 7d98cae on nipy:master.

@arokem

This comment has been minimized.

Member

arokem commented May 12, 2017

@ghoshbishakh

This comment has been minimized.

Member

ghoshbishakh commented May 12, 2017

Done

@coveralls

This comment has been minimized.

coveralls commented May 12, 2017

Coverage Status

Coverage decreased (-3.0%) to 85.607% when pulling f13afc4 on ghoshbishakh:docforcmd into 7d98cae on nipy:master.

1 similar comment
@coveralls

This comment has been minimized.

coveralls commented May 12, 2017

Coverage Status

Coverage decreased (-3.0%) to 85.607% when pulling f13afc4 on ghoshbishakh:docforcmd into 7d98cae on nipy:master.

@coveralls

This comment has been minimized.

coveralls commented May 14, 2017

Coverage Status

Coverage decreased (-3.0%) to 85.607% when pulling 2351eb6 on ghoshbishakh:docforcmd into 7d98cae on nipy:master.

1 similar comment
@coveralls

This comment has been minimized.

coveralls commented May 14, 2017

Coverage Status

Coverage decreased (-3.0%) to 85.607% when pulling 2351eb6 on ghoshbishakh:docforcmd into 7d98cae on nipy:master.

@Garyfallidis

This comment has been minimized.

Member

Garyfallidis commented Jun 13, 2017

Hey @ghoshbishakh this PR needs a rebase to get rid of the failing test. Then I believe we are good to merge if you agree.

@ghoshbishakh

This comment has been minimized.

Member

ghoshbishakh commented Jun 14, 2017

Yes Ill update it by tonight

@ghoshbishakh ghoshbishakh force-pushed the ghoshbishakh:docforcmd branch from ed98f54 to 12156a2 Jun 14, 2017

@coveralls

This comment has been minimized.

coveralls commented Jun 14, 2017

Coverage Status

Coverage remained the same at 85.656% when pulling 12156a2 on ghoshbishakh:docforcmd into 599af3e on nipy:master.

1 similar comment
@coveralls

This comment has been minimized.

coveralls commented Jun 14, 2017

Coverage Status

Coverage remained the same at 85.656% when pulling 12156a2 on ghoshbishakh:docforcmd into 599af3e on nipy:master.

@coveralls

This comment has been minimized.

coveralls commented Jun 14, 2017

Coverage Status

Coverage remained the same at 85.656% when pulling 12156a2 on ghoshbishakh:docforcmd into 599af3e on nipy:master.

1 similar comment
@coveralls

This comment has been minimized.

coveralls commented Jun 14, 2017

Coverage Status

Coverage remained the same at 85.656% when pulling 12156a2 on ghoshbishakh:docforcmd into 599af3e on nipy:master.

@skoudoro skoudoro merged commit f2919aa into nipy:master Jun 20, 2017

4 checks passed

codecov/patch Coverage not affected when comparing 599af3e...12156a2
Details
codecov/project 87.05% (+<.01%) compared to 599af3e
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage remained the same at 85.656%
Details

ShreyasFadnavis pushed a commit to ShreyasFadnavis/dipy that referenced this pull request Sep 20, 2018

Merge pull request nipy#1090 from ghoshbishakh/docforcmd
Documentation for command line interfaces
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment