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

Feature/issue 373 #397

Merged
merged 23 commits into from
Dec 12, 2017
Merged

Feature/issue 373 #397

merged 23 commits into from
Dec 12, 2017

Conversation

jbiggsets
Copy link
Collaborator

@jbiggsets jbiggsets commented Dec 7, 2017

This is a PR to address #373. It updates all of the docstrings in the skll package. There are two main types of changes:

  1. Converting existing docstrings to numpy-style. This sometimes involved adding additional information, such as any errors being raised or arguments the were not mentioned.
  2. Adding new numpy-style docstrings to functions and methods where none previously existed.

I have also added the sphinx.ext.napoleon extension to the doc/config.py file to properly parse numpy-style docstrings when generating documentation for RTD.

Please review new docstrings especially carefully, with an eye toward proper argument and return data types.

@jbiggsets jbiggsets added this to the 1.5 milestone Dec 7, 2017
@jbiggsets jbiggsets changed the title Feature/issue 373 [WIP] Feature/issue 373 Dec 7, 2017
@coveralls
Copy link

coveralls commented Dec 7, 2017

Coverage Status

Coverage remained the same at 92.432% when pulling b93e3cd on feature/issue-373 into 08e42ba on master.

@coveralls
Copy link

coveralls commented Dec 8, 2017

Coverage Status

Coverage remained the same at 92.414% when pulling 7776987 on feature/issue-373 into 47bfa4c on master.

@jbiggsets jbiggsets changed the title [WIP] Feature/issue 373 Feature/issue 373 Dec 8, 2017
@jbiggsets
Copy link
Collaborator Author

This is ready for review. @desilinguist, I'm no longer getting any warnings when I generate the documentation with make clean && make html locally.

Robert Pugh and others added 2 commits December 8, 2017 14:15
…com:EducationalTestingService/skll into feature/issue-373

Adding tests to ensure coverage is okay.
@coveralls
Copy link

coveralls commented Dec 11, 2017

Coverage Status

Coverage increased (+0.1%) to 92.523% when pulling cbf2c48 on feature/issue-373 into 47bfa4c on master.

@desilinguist
Copy link
Member

Okay, now that the coverage issue is resolved, I am going to start to review this.

skll/config.py Outdated
Returns
-------
invalid_options : set
The set of invalid options specified by the user.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we want to specify the type of the set elements to expect? eg set of str. This change would apply to a lot of other functions as well.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, this seems like a nice thing to do.

desilinguist and others added 4 commits December 11, 2017 14:02
- Replace `Note` with `Notes`.
- Fix some boolean defaults.
- Add more details about lists and sets.
- feature_set -> `FeatureSet`
- Some other minor changes
- Add new authors.
@coveralls
Copy link

coveralls commented Dec 11, 2017

Coverage Status

Coverage decreased (-0.8%) to 91.612% when pulling ac1002f on feature/issue-373 into 47bfa4c on master.

@coveralls
Copy link

coveralls commented Dec 11, 2017

Coverage Status

Coverage increased (+0.1%) to 92.523% when pulling a22177d on feature/issue-373 into 47bfa4c on master.

3 similar comments
@coveralls
Copy link

Coverage Status

Coverage increased (+0.1%) to 92.523% when pulling a22177d on feature/issue-373 into 47bfa4c on master.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.1%) to 92.523% when pulling a22177d on feature/issue-373 into 47bfa4c on master.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.1%) to 92.523% when pulling a22177d on feature/issue-373 into 47bfa4c on master.

- Use double backticks where possible to be consistent with the non-docstring documentation.
- Fix some typos here and there.
- Minor rephrasings.
- Some other minor changes.
@desilinguist
Copy link
Member

@aoifecahill Once the build passes, this is just waiting for your approval.

@coveralls
Copy link

coveralls commented Dec 12, 2017

Coverage Status

Coverage increased (+0.1%) to 92.523% when pulling 3ce6b55 on feature/issue-373 into 47bfa4c on master.

Copy link
Collaborator

@aoifecahill aoifecahill left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, thanks!

@desilinguist desilinguist merged commit 4db4392 into master Dec 12, 2017
@desilinguist desilinguist deleted the feature/issue-373 branch December 12, 2017 16:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants