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.

Sign up for GitHub

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

Jump to bottom

docs(project): Add docstrings to functions in project #541

Merged
merged 23 commits into from
Oct 22, 2024
Merged

docs(project): Add docstrings to functions in project #541

merged 23 commits into from
Oct 22, 2024

Conversation

MarieS-WiMLDS
Copy link
Contributor

@MarieS-WiMLDS MarieS-WiMLDS commented Oct 18, 2024 •
edited
Loading

Add docstrings to the functions in the project class.

@MarieS-WiMLDS MarieS-WiMLDS linked an issue Oct 18, 2024 that may be closed by this pull request
doc: improve the docstring on the project class #535
Closed
@MarieS-WiMLDS MarieS-WiMLDS marked this pull request as draft October 18, 2024 14:29
@MarieS-WiMLDS MarieS-WiMLDS changed the title (doc) add docstrings to functions in project docs(project): add docstrings to functions in project Oct 18, 2024
@MarieS-WiMLDS MarieS-WiMLDS changed the title docs(project): add docstrings to functions in project docs(project): Add docstrings to functions in project Oct 18, 2024
@MarieS-WiMLDS MarieS-WiMLDS marked this pull request as ready for review October 18, 2024 14:49
tuscland
tuscland requested changes Oct 18, 2024
View reviewed changes
Copy link
Member

@tuscland tuscland left a comment

Choose a reason for hiding this comment

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

Minor adjustments for more idiomatic english, simplifications, and one fix (view instead of item).

skore/src/skore/project.py Outdated Show resolved Hide resolved
skore/src/skore/project.py Outdated Show resolved Hide resolved
skore/src/skore/project.py Outdated Show resolved Hide resolved
skore/src/skore/project.py Outdated Show resolved Hide resolved
skore/src/skore/project.py Outdated Show resolved Hide resolved
skore/src/skore/project.py Outdated Show resolved Hide resolved
skore/src/skore/project.py Outdated Show resolved Hide resolved
skore/src/skore/project.py Show resolved Hide resolved
skore/src/skore/project.py Outdated Show resolved Hide resolved
skore/src/skore/project.py Outdated Show resolved Hide resolved
thomass-dev
thomass-dev requested changes Oct 21, 2024
View reviewed changes
skore/src/skore/project.py Outdated Show resolved Hide resolved
@thomass-dev thomass-dev mentioned this pull request Oct 21, 2024
Merged
augustebaum pushed a commit that referenced this pull request Oct 21, 2024
@thomass-dev
fix(ci): Fix ruff being skipped in CI by prefixing destinations with …
ef1bfec 
…`skore/` after #440 (#545)

We can see in #541 that pre-commit doesn't execute ruff:


![image](https://github.com/user-attachments/assets/3822a0e6-ca08-43a8-934b-0c417d6d7b6e)

Now:


![image](https://github.com/user-attachments/assets/c89394dd-5c8c-49d5-ba67-e05724505608)
tuscland
tuscland requested changes Oct 21, 2024
View reviewed changes
skore/src/skore/project.py Outdated Show resolved Hide resolved
augustebaum
augustebaum requested changes Oct 21, 2024
View reviewed changes
Copy link
Contributor

@augustebaum augustebaum left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Thanks for the added precision, this is really welcome. I made a few changes but I realize that not everything was previously agreed upon and we can discuss it. A few notable remarks:

  • The Attributes is for documenting classes, not functions. When a function returns an object of type A, it makes sense to just say it returns an A and document in a specific place what an A is, in depth
  • Each section of the docstring needs to be separated from the others by a blank line (that's for the docs generator to understand that a new paragraph is starting)
  • The Returns block already explains what is returned, so writing "Returns ..." in the description is redundant (see numpydoc spec)
  • The Raises block already says which exceptions can be raised, so writing "Raises a KeyError" in the description is redundant (see numpydoc spec)

skore/src/skore/project.py Outdated Show resolved Hide resolved
skore/src/skore/project.py Outdated Show resolved Hide resolved
skore/src/skore/project.py Show resolved Hide resolved
skore/src/skore/project.py Outdated Show resolved Hide resolved
skore/src/skore/project.py Outdated Show resolved Hide resolved
skore/src/skore/project.py Outdated Show resolved Hide resolved
skore/src/skore/project.py Outdated Show resolved Hide resolved
skore/src/skore/project.py Outdated Show resolved Hide resolved
skore/src/skore/project.py Outdated Show resolved Hide resolved
skore/src/skore/project.py Outdated Show resolved Hide resolved
MarieS-WiMLDS and others added 10 commits October 21, 2024 16:16
@MarieS-WiMLDS @augustebaum
Update skore/src/skore/project.py
5331889 
Co-authored-by: Auguste Baum <52001167+augustebaum@users.noreply.github.com>
@MarieS-WiMLDS @augustebaum
Update skore/src/skore/project.py
cf043c3 
Co-authored-by: Auguste Baum <52001167+augustebaum@users.noreply.github.com>
@MarieS-WiMLDS @augustebaum
Update skore/src/skore/project.py
d4494d2 
Co-authored-by: Auguste Baum <52001167+augustebaum@users.noreply.github.com>
@MarieS-WiMLDS @augustebaum
Update skore/src/skore/project.py
7e79250 
Co-authored-by: Auguste Baum <52001167+augustebaum@users.noreply.github.com>
@MarieS-WiMLDS @augustebaum
Update skore/src/skore/project.py
e6d1671 
Co-authored-by: Auguste Baum <52001167+augustebaum@users.noreply.github.com>
@MarieS-WiMLDS @augustebaum
Update skore/src/skore/project.py
57e098f 
Co-authored-by: Auguste Baum <52001167+augustebaum@users.noreply.github.com>
@MarieS-WiMLDS @augustebaum
Update skore/src/skore/project.py
7d17197 
Co-authored-by: Auguste Baum <52001167+augustebaum@users.noreply.github.com>
@MarieS-WiMLDS @augustebaum
Update skore/src/skore/project.py
215251e 
Co-authored-by: Auguste Baum <52001167+augustebaum@users.noreply.github.com>
@MarieS-WiMLDS
(doc) project: returns to get in func description
0205ff0
@MarieS-WiMLDS
(doc) project: concise list_keys description
20e7837
@MarieS-WiMLDS @augustebaum
Update skore/src/skore/project.py
8ebf15c 
Co-authored-by: Auguste Baum <52001167+augustebaum@users.noreply.github.com>
MarieS-WiMLDS and others added 6 commits October 21, 2024 18:15
@MarieS-WiMLDS @augustebaum
Update skore/src/skore/project.py
c536310 
Co-authored-by: Auguste Baum <52001167+augustebaum@users.noreply.github.com>
@MarieS-WiMLDS @augustebaum
Update skore/src/skore/project.py
1962c41 
Co-authored-by: Auguste Baum <52001167+augustebaum@users.noreply.github.com>
@MarieS-WiMLDS @augustebaum
Update skore/src/skore/project.py
71db3ee 
Co-authored-by: Auguste Baum <52001167+augustebaum@users.noreply.github.com>
@MarieS-WiMLDS
Merge branch 'main' into 535-docstring_project
aa08266
@MarieS-WiMLDS
Merge branch 'main' into 535-docstring_project
6d1958e
@MarieS-WiMLDS
(doc) project: remore trailing whitespace
a7def51
augustebaum
augustebaum approved these changes Oct 22, 2024
View reviewed changes
Copy link
Contributor

@augustebaum augustebaum left a comment

Choose a reason for hiding this comment

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

Nice work!

@thomass-dev thomass-dev merged commit 20721cf into main Oct 22, 2024
4 checks passed
@thomass-dev thomass-dev deleted the 535-docstring_project branch October 22, 2024 09:53
@sylvaincom sylvaincom mentioned this pull request Oct 28, 2024
Closed
2 tasks
thomass-dev added a commit that referenced this pull request Dec 2, 2024
@thomass-dev
fix(ci): Fix ruff being skipped in CI by prefixing destinations with …
8a1857c 
…`skore/` after #440 (#545)

We can see in #541 that pre-commit doesn't execute ruff:


![image](https://github.com/user-attachments/assets/3822a0e6-ca08-43a8-934b-0c417d6d7b6e)

Now:


![image](https://github.com/user-attachments/assets/c89394dd-5c8c-49d5-ba67-e05724505608)
thomass-dev added a commit that referenced this pull request Dec 2, 2024
@MarieS-WiMLDS @augustebaum @thomass-dev
docs(project): Add docstrings to functions in project (#541)
c2be216 
Add docstrings to the functions in the project class.

---------

Co-authored-by: Auguste Baum <52001167+augustebaum@users.noreply.github.com>
Co-authored-by: Thomas S. <thomas@probabl.ai>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@augustebaum augustebaum augustebaum approved these changes

@thomass-dev thomass-dev thomass-dev approved these changes

@sylvaincom sylvaincom Awaiting requested review from sylvaincom

@tuscland tuscland Awaiting requested review from tuscland

Assignees

@MarieS-WiMLDS MarieS-WiMLDS

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

doc: improve the docstring on the project class
4 participants
@MarieS-WiMLDS @tuscland @thomass-dev @augustebaum