Skip to content
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

Update command line documentation generation #1800

Merged
merged 3 commits into from May 14, 2019

Conversation

@skoudoro
Copy link
Member

commented Mar 28, 2019

Description

this PR should fix @arokem comment concerning the command line documentation.
Reference: #1793

  • Maybe we need to "cherry-pick" on maint/0.16.x branch and regenerate the documentation after merging.

  • We need a new PR to reformat this docstring. It is not beautiful for the moment.

Result Example:

image

image

@skoudoro skoudoro changed the title update doc cmd line generation Update command line documentation generation Mar 28, 2019

@arokem
Copy link
Member

left a comment

This looks pretty good to me!

One thing I noticed is that the first word in "positional arguments" and "optional arguments" should be capitalized. Where are these?

Regarding what to do now: I suggest that we fix up #1794, providing the missed back-compatibility and then put out a quick 0.16.1 with this included in the documentation. What do you think about that?

for name, obj in members
if inspect.isclass(obj) and
issubclass(obj, workflow_module.Workflow) and
name not in ['Workflow', 'CombinedWorkflow']

This comment has been minimized.

Copy link
@arokem

arokem Mar 29, 2019

Member

Maybe put this list somewhere more prominent as a module-level constant? In case we need to find it and add more items in the future?

This comment has been minimized.

Copy link
@skoudoro

skoudoro Apr 2, 2019

Author Member

Good idea, will do, Thanks!

@skoudoro

This comment has been minimized.

Copy link
Member Author

commented Apr 2, 2019

One thing I noticed is that the first word in "positional arguments" and "optional arguments" should be capitalized. Where are these?

They are on this line. I really think that we should reformat the output but I will do it on a new PR

Regarding what to do now: I suggest that we fix up #1794,

It seems to be already fix and on nipype master. they used a alias (from ... import ... as ...)

and then put out a quick 0.16.1 with this included in the documentation

I do not think we need a quick 0.16.1 since we will start to promote the command line only after the 1.0.0. We still have to improve and fix some workflows

@codecov-io

This comment has been minimized.

Copy link

commented Apr 3, 2019

Codecov Report

Merging #1800 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #1800   +/-   ##
=======================================
  Coverage   83.59%   83.59%           
=======================================
  Files         118      118           
  Lines       14486    14486           
  Branches     2283     2283           
=======================================
  Hits        12109    12109           
  Misses       1851     1851           
  Partials      526      526

@skoudoro skoudoro added this to the 1.0 milestone Apr 23, 2019

@@ -13,6 +13,8 @@
# version comparison
from distutils.version import LooseVersion as V

# List of workflows to ignore
SKIP_WORKFLOWS_LIST = ['Workflow', 'CombinedWorkflow']

This comment has been minimized.

Copy link
@Borda

Borda Apr 27, 2019

Contributor

use rather tuple as this list should no change
SKIP_WORKFLOWS_LIST = ('Workflow', 'CombinedWorkflow')

def get_help_string(class_obj):
# return inspect.getdoc(class_obj.run)
try:
ia_module = importlib.import_module("dipy.workflows.base")

This comment has been minimized.

Copy link
@Borda

Borda Apr 27, 2019

Contributor

why you cannot use ia_module = dipy.workflows.base ?

This comment has been minimized.

Copy link
@skoudoro

skoudoro Apr 29, 2019

Author Member

Hi @Borda, Thank you for your review.

I do not understand your point here and how it will work without importing the module. Can you give me more detail?

This comment has been minimized.

Copy link
@Borda

Borda Apr 29, 2019

Contributor

the question is why are you using ia_module = importlib.import_module("dipy.workflows.base") instead of ia_module = dipy.workflows.base ?

This comment has been minimized.

Copy link
@skoudoro

skoudoro Apr 29, 2019

Author Member

I need to import the module. if I do ia_module = dipy.workflows.base the module is not imported right or I miss something?

This comment has been minimized.

Copy link
@Borda

Borda May 14, 2019

Contributor

what do you mean by right imported? import dipy.workflows.base works firn for me...


workflow_desc = {}
# We get all workflows class obj in a dictionnary
for f in os.listdir(pjoin('..', 'dipy', 'workflows')):

This comment has been minimized.

Copy link
@Borda

Borda Apr 27, 2019

Contributor

rather use a longer name which is more describtive f -> path_file

cmd_list = []
for fpath in workflow_flist:
fname = os.path.basename(fpath)
with open(fpath) as f:

This comment has been minimized.

Copy link
@Borda

Borda Apr 27, 2019

Contributor

rather use at least characters f -> fp as file pointer


flow_name = list(flow_name)[-1]
print("Generating docs for: {0} ({1})".format(fname, flow_name))
out_f = fname + ".rst"

This comment has been minimized.

Copy link
@Borda

Borda Apr 27, 2019

Contributor

rather out_fname

@skoudoro skoudoro force-pushed the skoudoro:fix-utility-reference-doc branch from 5adc22f to 24a49db May 7, 2019

@skoudoro skoudoro force-pushed the skoudoro:fix-utility-reference-doc branch from 24a49db to adee03f May 9, 2019

@skoudoro

This comment has been minimized.

Copy link
Member Author

commented May 13, 2019

What is the final decision concerning this PR @arokem?

@arokem

This comment has been minimized.

Copy link
Member

commented May 14, 2019

Looks like some of @Borda's comments are still not addressed?

@skoudoro

This comment has been minimized.

Copy link
Member Author

commented May 14, 2019

they are addressed on adee03f . Any opinion @Borda ?

@Borda

This comment has been minimized.

Copy link
Contributor

commented May 14, 2019

Hi, I made it as comments, not as requirements :) most of my comments were to the PEP8 and clarity of code except the import dipy.workflows.base

@arokem

This comment has been minimized.

Copy link
Member

commented May 14, 2019

Oh sorry - I didn't notice that these comments were marked "outdated". Merging.

@arokem arokem merged commit 8379bbd into nipy:master May 14, 2019

5 checks passed

Codacy/PR Quality Review Up to standards. A positive pull request.
Details
codecov/patch Coverage not affected when comparing 977ea03...adee03f
Details
codecov/project 83.59% remains the same compared to 977ea03
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@skoudoro skoudoro deleted the skoudoro:fix-utility-reference-doc branch Aug 7, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.