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

Add Docstring section to development docs #2138

Merged
merged 6 commits into from
Oct 31, 2023
Merged

Conversation

zm711
Copy link
Collaborator

@zm711 zm711 commented Oct 28, 2023

A companion documentation section to go with @alejoe91 #2134. This way future developers will be adequately warned about what is expected in the docstrings.

Read through the docstring section and let me know if you want me to include more info.

Other minor changes
*I restylized black to Black since that is their official name whereas black is the package (similar to this repo :) )
* added a note about fmt skip vs fmt off for black
* couple typo fixes

One question is near the bottom the section originally said edit the travis.yml, but I assumed it was suppose to be edit the pyproject to add the dependencies to the test section, but if I'm wrong correct that section.

also fix some typos
@zm711 zm711 added the documentation Improvements or additions to documentation label Oct 28, 2023
Copy link
Collaborator

@h-mayorquin h-mayorquin left a comment

Choose a reason for hiding this comment

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

This looks good to me. I did this the first time and this is an improvement upon it. I have been meaning to add @alejoe91 convention for a while so I am thankful that @zm711 decided to do this.

I added two small suggestions that are up to @zm711 to follow up or not.

doc/development/development.rst Show resolved Hide resolved
doc/development/development.rst Show resolved Hide resolved
@zm711
Copy link
Collaborator Author

zm711 commented Oct 29, 2023

While you're checking this out @h-mayorquin. I know you added the datalad instructions but I for one can't get datalad working on Mac ( m-series). I think there is a mismatch between datalad, git-annex, and brew. I'll update those instructions if I can figure it out, but just wanted that on your radar in case you know any magic tips for getting it to work.

@h-mayorquin
Copy link
Collaborator

@zm711 I was kind of proud that I got datalad working for all the three OSs in the CI at some point in time. This was accomplished using those instructions. Because of the Sam moratorium on testing we ended up not using that but ... flawed human as I am, I wanted to keep those instructions somewhere and they ended up there.

In retrospective, this is wrong. We are doing duplication by keeping those instructions there and, even worse, making promises that we don't know if we can keep ("the method works"). We should just point out to the documentation in datalad and be done with it.

Plus, now that we have good generators we will be getting rid of Datalad for core testing (which is great!) so this is even less necessary as we will only be needing if for extractor testing.

@zm711
Copy link
Collaborator Author

zm711 commented Oct 29, 2023

First off I always super appreciate all your hard work getting the ci working @h-mayorquin!

Plus, now that we have good generators we will be getting rid of Datalad for core testing (which is great!) so this is even less necessary as we will only be needing if for extractor testing.

I agree. I think switching the examples to the generators (in the modules gallery) would be super beneficial. Because right now we allow people to download the scripts, but the scripts will fail if the user doesn't have datalad.

We should just point out to the documentation in datalad and be done with it.

I'll change the datalad section right now just so that we don't "over-promise".

(I was able to get datalad with windows, but I had to specify my git-annex version in the datalad installer and jump through a couple hoops, so yeah I think the packages are just changing at different rates, which makes it hard to have the instructions just work).

Also just as an fyi (since you worked so hard on the ci). I recently tried to run the whole test-suite on windows (I ended about 7/8 of the way through) and ended up with something like 50-60 test failures. Mostly due to shutil.rmtree being blocked by Windows permssion errors.

@h-mayorquin
Copy link
Collaborator

Thanks for testing this on windows.

In the last meeting we discussed testing. I am hoping that by the end of this week and next week I will be working in some outstanding* maintenance issues which will include testing.

(I just realized that this is synonym with great which I find strange)

@alejoe91 alejoe91 added this to the 0.99.0 milestone Oct 30, 2023
@alejoe91 alejoe91 merged commit fabccac into SpikeInterface:main Oct 31, 2023
9 checks passed
@zm711 zm711 deleted the dev-docs branch November 17, 2023 15:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants