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

TST/DOC: add tests and use new docstring style for class OnyoRepo #394

Merged
merged 8 commits into from
Jul 31, 2023

Conversation

TobiasKadelka
Copy link
Collaborator

@TobiasKadelka TobiasKadelka commented Jul 20, 2023

Changes:

  • adds tests to some functions of OnyoRepo
  • adds or normalizes doc-strings for those functions (based on discussion in numpy-ify doctstrings #339)

@TobiasKadelka TobiasKadelka marked this pull request as draft July 20, 2023 10:29
@codecov
Copy link

codecov bot commented Jul 20, 2023

Codecov Report

Patch coverage: 100.00% and project coverage change: +0.21% 🎉

Comparison is base (e9e499a) 94.58% compared to head (ff65fff) 94.79%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #394      +/-   ##
==========================================
+ Coverage   94.58%   94.79%   +0.21%     
==========================================
  Files          51       51              
  Lines        3990     4037      +47     
  Branches      947      953       +6     
==========================================
+ Hits         3774     3827      +53     
+ Misses        121      117       -4     
+ Partials       95       93       -2     
Files Changed Coverage Δ
onyo/lib/onyo.py 90.97% <100.00%> (+2.77%) ⬆️
onyo/lib/tests/test_onyo.py 100.00% <100.00%> (ø)

... and 1 file with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@TobiasKadelka TobiasKadelka changed the title TST: add tests for class OnyoRepo TST/DOC: add tests and docstrings for class OnyoRepo Jul 24, 2023
Gives the function a new docstring in the style discussed in issue psyinfra#339.
Also normalizes type hints e.g. usage of `Optional`, and other code style aspects.
Adds a docstring in the style discussed in psyinfra#339.
This test modifies an asset in OnyoRepo with an external function
so that the cache is not up-to-date, and then tests the function
`OnyoRepo.clear_caches()` to re-load the cache.
@TobiasKadelka TobiasKadelka marked this pull request as ready for review July 26, 2023 09:48
@TobiasKadelka TobiasKadelka changed the title TST/DOC: add tests and docstrings for class OnyoRepo TST/DOC: add tests and use new docstring style for class OnyoRepo Jul 27, 2023
Copy link
Collaborator

@bpoldrack bpoldrack 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 overall, left minor suggestions/coments.

Gives the function a new docstring in the style discussed in issue psyinfra#339.
The test modifies an OnyoRepo with the python implementations of the
commands and then calls `generate_commit_message()` and checks the
default commit message that was generated.

There is the idea to change the commit message body in a future PR.
This would impact parts of the test function written now, and I assume
the current test should help as a starting point to test the new format.
I add a doc-string to the function and test for the specified
definition of it.
A collection of small changes, that does not effect the code,
and that did not fit into the other commits.

Applies for some functions the new docstring style.

Some changes are also just about writting style, that I figured
out too late to include in the other commits before.
I update the doc-string of the function to the new style, and change
the type hints, because this function should just recive a string.

Adds also tests to the function for valid and invalid calls.
@bpoldrack bpoldrack merged commit a1c4aff into psyinfra:main Jul 31, 2023
@bpoldrack
Copy link
Collaborator

Thanks!

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.

2 participants