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

[Testing]: check docstrings for consistency #284

Closed
2 tasks done
CodyCBakerPhD opened this issue Jan 23, 2023 · 8 comments
Closed
2 tasks done

[Testing]: check docstrings for consistency #284

CodyCBakerPhD opened this issue Jan 23, 2023 · 8 comments

Comments

@CodyCBakerPhD
Copy link
Member

What would you like to see added to NeuroConv?

Replaces the dynamic parsing proposal of #278

Add simple tests that check the docstring description of both upstream arguments that are copied and propagated as well as internal repeated arguments

very low priority, however

Is your feature request related to a problem?

No response

Do you have any interest in helping implement the feature?

Yes.

Code of Conduct

@h-mayorquin
Copy link
Collaborator

Does the recently merged #771 relates to this?

@CodyCBakerPhD
Copy link
Member Author

#771 leverages any consistency already there, but doesn't actually check for it and would benefit from this

Pydocstyle in #276 would be sort of first step, then beyond that possible what Paul did in ROIExtractors CI

@h-mayorquin
Copy link
Collaborator

Maybe @pauladkisson would like to take on this one then given he has some experience already.

@CodyCBakerPhD
Copy link
Member Author

@h-mayorquin Are you putting your foot down on this one as being too annoying? Note that it is a feature utilized by the schema inference (and thus adds descriptions to all fields on the GUIDE without manual shema injections at the class level)

@h-mayorquin
Copy link
Collaborator

Let's discuss this in our meeting. It is more that I have not had the time and the priority is low.

@h-mayorquin
Copy link
Collaborator

h-mayorquin commented Aug 20, 2024

After discussion on 2024/08/20

These could be three things

  • First this started as a series of checks to ensure that repeated arguments (e.g. the description of verbose) are consistent across all uses on the database.
  • The docstring exists.
  • Consistency of style (do we comply with numpy style).

The relationship with the guide is that it uses the descriptions in the json schema. So they being consistent helps but should not be a concern here.

@h-mayorquin
Copy link
Collaborator

First this started as a series of checks to ensure that repeated arguments (e.g. the description of verbose) are consistent across all uses on the database.

@pauladkisson pointed out in the meeting of 2024-08-27 that there might not be a way to automatize this in a resaonble way. The main reason is that maybe some docstrings should be different even if they cover the same behavior if they can add context that is relevant on that specific case (e.g. verbose prints different things in different interfaces and the docstring could describe what it does on each interface).

I think that the genesis of the consistency idea was when were thinking about conversion options and we thought that they should be more or less the same everywhere. Maybe that assumption was wrong for the ssame reasons that @pauladkisson points out. Anyway, I am happy to mark this as closed once #1029 and #1028 are merge.

@pauladkisson
Copy link
Member

Closing this since the relevant PRs have been closed.

@pauladkisson pauladkisson closed this as not planned Won't fix, can't repro, duplicate, stale Sep 18, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants