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

Relax docstrings #19

Merged
merged 31 commits into from
Aug 27, 2024
Merged

Relax docstrings #19

merged 31 commits into from
Aug 27, 2024

Conversation

pauladkisson
Copy link
Member

Fixes #17

tests/conftest.py Outdated Show resolved Hide resolved
Copy link

@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.

I did not see this before. Left some comments.

Choose a reason for hiding this comment

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

Shouldn't be this on the roiextractors repo?

Copy link
Member Author

Choose a reason for hiding this comment

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

It's easier to test that the action works properly when it's on the same repo.

Choose a reason for hiding this comment

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

I don't understand, so why not have the neuroconv one here as well?

I thought that the point of having the action here instead of the repos was so we could re-use them across the organization.

Copy link
Member

Choose a reason for hiding this comment

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

The action is what is centralized in catalystneuro/.github

The workflows are what calls that action

The workflows here are 'tests' or 'demos' of the actions here for debugging purposes

We could theoretically add 'neuroconv' here as an extra 'test case'

But the actual workflows used to assess PRs should live on their respective repos (b/c no other way to add to PR requirements, show up on repo dashboards, etc)

Choose a reason for hiding this comment

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

The workflows here are 'tests' or 'demos' of the actions here for debugging purposes

OK. I did not get that it was meant to be a test of the action from @pauladkisson response above. I think the test could be simpler (like using a simpler repo where the failures would make sense to someone that is not familiar with roiextractors) though but that's probably too much work.

Copy link
Member

Choose a reason for hiding this comment

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

It's true, a dedicated test repo with simple demo cases and documentation of expected results would be nice

But you are correct it would take some effort

.github/actions/check_docstrings/action.yml Outdated Show resolved Hide resolved
.github/actions/check_docstrings/action.yml Outdated Show resolved Hide resolved
tests/conftest.py Show resolved Hide resolved
Copy link

@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.

Some finall suggestions to update the actions that we rely on, documentation and to change the test to use relative paths so we can test the local commit.

.github/actions/check_docstrings/action.yml Outdated Show resolved Hide resolved
.github/actions/check_docstrings/action.yml Outdated Show resolved Hide resolved
.github/workflows/test_check_docstrings.yaml Outdated Show resolved Hide resolved
.github/workflows/test_check_docstrings.yaml Show resolved Hide resolved
.github/actions/check_docstrings/action.yml Outdated Show resolved Hide resolved
.github/actions/check_docstrings/action.yml Outdated Show resolved Hide resolved
.github/actions/check_docstrings/action.yml Outdated Show resolved Hide resolved
@pauladkisson
Copy link
Member Author

Some final suggestions to update the actions that we rely on, documentation and to change the test to use relative paths so we can test the local commit.

Done!

Copy link

@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.

LGTM.

@pauladkisson pauladkisson merged commit 0471705 into main Aug 27, 2024
@pauladkisson pauladkisson deleted the relax_docstrings branch August 27, 2024 17:12
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.

relax docstring requirements
3 participants