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

Docs: render autodoc & typehints in the API documentation #359

Merged
merged 27 commits into from
Jan 17, 2024
Merged

Conversation

anilbey
Copy link
Contributor

@anilbey anilbey commented Jan 16, 2024

Description

Docs now show all of the public functions in the API. Before it was only listing their names.
Docs show the argument types and return types. API docs are type checked using type hints and a static type checker to ensure they are always up-to-date.
Docs show CHANGELOG.
A few fixes in the function/method docstrings.

Before

Only the function names and the first line of docstring are provided.

docs-screenshot-before

After

Not only the first line but the complete docstring get generated for each function. Return types and argument types are annotated using type hints (instead of using docstrings).

docs-screenshot-after

Checklist:

  • Unit tests are added to cover the changes.
  • The changes are mentioned in the documentation.
  • CHANGELOG.md is updated (skip if the change is not important for the changelog).

@anilbey anilbey self-assigned this Jan 16, 2024
@anilbey anilbey marked this pull request as ready for review January 17, 2024 10:16
warnings.warn(
"Error while calculating feature %s: %s" %
(featureName, cppcore.getgError()),
f"Error while calculating {feature_name}, {cppcore.getgError()}",
RuntimeWarning)
return None
else:
Copy link
Collaborator

Choose a reason for hiding this comment

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

can remove the else: here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Coming with the next push.

Seeks for the stim_start and stim_end parameters inside the Neo data.

Args:
blocks (Neo object blocks): Description of what blocks represents.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Could you describe what blocks represent here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This function is written 7 year ago using the Neo package at that time. There is an internal efel ticket to update the usage to the recent one. There is a high chance that this function will be written in a much simpler way. Shall we update the args once that ticket it through?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Alright, sure, no problem

Copy link
Collaborator

Choose a reason for hiding this comment

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

Noted, I will make the update when I work on this ticket

efel/io.py Outdated
The returned object is presented like this :
returned object : [Segments_1, Segments_2, ..., Segments_n]
Segments_1 = [Traces_1, Traces_2, ..., Traces_n]
Seeks for the stim_start and stim_end parameters inside the Neo data.
Copy link
Collaborator

Choose a reason for hiding this comment

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

It looks like you just copy/pasted docstring from previous function here. Could you correct it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh good that you noticed. This docstring and the one above were both raising sphinx errors, I must have copied the wrong one.

efel/api.py Outdated
@@ -1,10 +1,10 @@
from __future__ import annotations
Copy link
Collaborator

Choose a reason for hiding this comment

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

why put these import statements above the docstring?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is being updated in the next push

efel/api.py Outdated
path to the location of a Dependency file
"""
eFEL uses 'Dependency' files to let the user define versions of features to use.
The installation directory of the eFEL contains a default 'DependencyV5.txt' file.
Copy link
Collaborator

Choose a reason for hiding this comment

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

the eFEL -> eFEL

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Next push

efel/api.py Outdated
True if feature_name exists, otherwise False
"""
def feature_name_exists(feature_name: str) -> bool:
"""Returns True if the feature name exists in the eFEL, False otherwise."""
Copy link
Collaborator

Choose a reason for hiding this comment

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

the eFEL -> eFEL

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Coming with the next push...

efel/api.py Outdated
The value is None if an error occured during the
calculation of the feature.
Args:
traces: Every trace dict represent one trace. The dict should have the
Copy link
Collaborator

Choose a reason for hiding this comment

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

typo: represent -> represents

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍

efel/api.py Outdated
raise_warnings: Raise warning when efel c++ returns an error

Returns:
For every input trace a feature value dict is return (in
Copy link
Collaborator

Choose a reason for hiding this comment

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

typo: return -> returned

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍

efel/api.py Outdated
calculation of the feature, or if the feature value array
was empty.
Args:
traces: Every trace dict represent one trace. The dict should have the
Copy link
Collaborator

Choose a reason for hiding this comment

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

typo: represent -> represents

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍

efel/api.py Outdated
raise_warnings: Raise warning when efel c++ returns an error

Returns:
For every input trace a feature value dict is return (in
Copy link
Collaborator

Choose a reason for hiding this comment

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

typo: is return -> is returned

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍

efel/api.py Show resolved Hide resolved
Copy link
Collaborator

@AurelienJaquier AurelienJaquier 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 to me but you might have some additional changes to make after pulling master hehehe

Copy link
Collaborator

@AurelienJaquier AurelienJaquier left a comment

Choose a reason for hiding this comment

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

alright, looks good!

@anilbey
Copy link
Contributor Author

anilbey commented Jan 17, 2024

@AurelienJaquier ah yes, mypy asked to handle the possible None values returned from get_cpp_feature for phaseslope_max.

Copy link
Collaborator

@ilkilic ilkilic left a comment

Choose a reason for hiding this comment

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

Looks great to me!

@anilbey anilbey merged commit 49a3bab into master Jan 17, 2024
9 checks passed
@anilbey anilbey deleted the docs branch January 17, 2024 12:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants