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

Make docstrings-style compliant and add assert messaging #241

Merged
merged 16 commits into from
Dec 5, 2023

Conversation

zm711
Copy link
Contributor

@zm711 zm711 commented Nov 28, 2023

So I switched most of the docstrings over to the SI format (I'm sure I'm missed some though) and I tried to add assert messaging to help users.

@zm711
Copy link
Contributor Author

zm711 commented Nov 28, 2023

The tests I broke were designed to prevent users from messing up. I'll take a look.

for test_io: is should probably be a type error and not a value error. I switched the pytest raises

for contact shapes I fed in the correct contact shape params.
@codecov-commenter
Copy link

codecov-commenter commented Nov 28, 2023

Codecov Report

Attention: 12 lines in your changes are missing coverage. Please review.

Comparison is base (70d77be) 88.90% compared to head (ea24068) 88.92%.

Files Patch % Lines
src/probeinterface/probe.py 68.57% 11 Missing ⚠️
src/probeinterface/io.py 66.66% 1 Missing ⚠️

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #241      +/-   ##
==========================================
+ Coverage   88.90%   88.92%   +0.02%     
==========================================
  Files          10       10              
  Lines        1694     1698       +4     
==========================================
+ Hits         1506     1510       +4     
  Misses        188      188              

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

@zm711
Copy link
Contributor Author

zm711 commented Nov 28, 2023

So one of the test failures was because I changed the type of error from Value to Type. I think it is a TypeError so I just changed that test.

The other set of failures were due to the fixture not specifying the contact_shape_params. Since the shape params are specific for the contact shape I think having an assert to prevent a user from doing this wrong is actually important, so I added the correct shape parameters for the fixture, but if you want me to revert that fixture and delete the assert confirming the correct mix of contact shape and contact shape params I could do that as well.

Coverage drop is just because I made some asserts multiple lines and not all asserts are being tested.

src/probeinterface/generator.py Outdated Show resolved Hide resolved
src/probeinterface/generator.py Outdated Show resolved Hide resolved
@zm711
Copy link
Contributor Author

zm711 commented Nov 29, 2023

Okay I'm all done with everything -- I won't push anything unless I get feedback.

As a reminder (to check super carefully):
the assert I added in the generator (do you want me to delete and revert the test or do you think it is a good idea to prevent users from specifying the wrong shape params--also not sure if there is a specific spikeinterface test that could be affected by this change).

@alejoe91
Copy link
Member

Thanks @zm711

I'll review between today and tomorrow!

Copy link
Member

@alejoe91 alejoe91 left a comment

Choose a reason for hiding this comment

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

Thanks @zm711

I requested a small change regarding the use of | and list[] for typing, which I believe needs from __future__ import annotations.

Could you add that when needed?

@zm711
Copy link
Contributor Author

zm711 commented Dec 5, 2023

Just to be safe I added future to all files (like Heberto has said, it's easy to miss that since we only test on 3.10), so since ProbeInterface is a pretty small code base I think it safer to just have from future everywhere and then we can delete it later.

I also added .ds_store to the gitignore since I do some work on Macs and don't want to accidentally upload a bunch of folder organization files :)

coverage failed which caused the test to fail. Could you try just re-running the test :)

@alejoe91 alejoe91 added documentation Improvements or additions to documentation and removed documentation Improvements or additions to documentation labels Dec 5, 2023
@alejoe91 alejoe91 merged commit c68204c into SpikeInterface:main Dec 5, 2023
3 checks passed
@zm711 zm711 deleted the docstrings branch December 5, 2023 11:33
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