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
Show file tree
Hide file tree
Changes from 16 commits
Commits
Show all changes
31 commits
Select commit Hold shift + click to select a range
8de751c
added names-to-skip to conftest
pauladkisson Aug 23, 2024
55c2daa
passed names-to-skip along through traversal methods
pauladkisson Aug 23, 2024
f254dc8
added names-to-skip to workflows
pauladkisson Aug 23, 2024
be55af5
corrected names
pauladkisson Aug 23, 2024
40a9b4b
added branch option
pauladkisson Aug 23, 2024
a4279f4
converted check_docstrings.yaml to an action
pauladkisson Aug 23, 2024
dc99769
moved actions folder outside .github
pauladkisson Aug 23, 2024
81cf696
testing actions.yaml
pauladkisson Aug 23, 2024
70f03e2
testing action.yml
pauladkisson Aug 23, 2024
224b081
testing action.yml
pauladkisson Aug 23, 2024
624e2a3
shell: bash
pauladkisson Aug 23, 2024
87d9768
moved to folder organization
pauladkisson Aug 23, 2024
21fe935
moved to folder organization
pauladkisson Aug 23, 2024
52b3aa1
updated folder organization
pauladkisson Aug 23, 2024
ac986e3
Merge branch 'main' into relax_docstrings
pauladkisson Aug 23, 2024
43029b5
rm actions folder
pauladkisson Aug 23, 2024
12e9d5b
added explicit condition for names_to_skip
pauladkisson Aug 26, 2024
f1c2fa9
removed setup conda
pauladkisson Aug 26, 2024
2a4b0bf
added call_check_docstrings back for testing (dont forget to remove b…
pauladkisson Aug 26, 2024
7c29271
@relax_docstrings
pauladkisson Aug 26, 2024
2806653
removed checkout home repo
pauladkisson Aug 26, 2024
a5d9ecc
readded checkout home repo
pauladkisson Aug 26, 2024
2c627e8
reorganized tests inside action directory
pauladkisson Aug 26, 2024
d19f0b1
added path to checkout
pauladkisson Aug 26, 2024
406559d
added path to checkout
pauladkisson Aug 26, 2024
9427950
added path to checkout
pauladkisson Aug 26, 2024
b430c60
added path to checkout
pauladkisson Aug 26, 2024
f8df9c3
added path to checkout
pauladkisson Aug 26, 2024
092d3e9
renamed call_check_docstrings to test_check_docstrings
pauladkisson Aug 26, 2024
06751b4
updated versions and added examples
pauladkisson Aug 27, 2024
e69bcf3
switched to local action
pauladkisson Aug 27, 2024
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
63 changes: 63 additions & 0 deletions .github/actions/check_docstrings/action.yml
Original file line number Diff line number Diff line change
@@ -0,0 +1,63 @@
name: Check Docstrings
inputs:
python-version:
description: 'The version of Python to use for the workflow.'
default: '3.10'
pauladkisson marked this conversation as resolved.
Show resolved Hide resolved
required: false
type: string
repository:
description: 'The repository to check the docstrings for.'
pauladkisson marked this conversation as resolved.
Show resolved Hide resolved
required: True
type: string
branch:
description: 'The branch to check the docstrings for.'
required: false
type: string
default: ''
CodyCBakerPhD marked this conversation as resolved.
Show resolved Hide resolved
package-name:
description: 'The name of the package to check the docstrings for.'
required: True
type: string
names-to-skip:
description: 'The names of the modules/classes/functions/methods to skip in the docstring check.'
pauladkisson marked this conversation as resolved.
Show resolved Hide resolved
required: false
type: string
default: ''

runs:
using: "composite"
steps:
- name: Setup Conda
uses: s-weigand/setup-conda@v1
pauladkisson marked this conversation as resolved.
Show resolved Hide resolved

- name: Setup Python ${{ inputs.python-version }}
uses: actions/setup-python@v2
pauladkisson marked this conversation as resolved.
Show resolved Hide resolved
with:
python-version: ${{ inputs.python-version }}

- name: Global Setup
run: |
pip install -U pip
pip install pytest-xdist
git config --global user.email "[email protected]"
git config --global user.name "CI Almighty"
shell: bash

- name: Checkout Repository to be tested
uses: actions/checkout@v2
pauladkisson marked this conversation as resolved.
Show resolved Hide resolved
with:
repository: ${{ inputs.repository }}
ref: ${{ inputs.branch }}

- name: Install package
run: pip install .
shell: bash

- name: Checkout Home Repository
pauladkisson marked this conversation as resolved.
Show resolved Hide resolved
uses: actions/checkout@v2
with:
repository: catalystneuro/.github

- name: Run docstring check
run: pytest tests/test_docstrings.py --package=${{ inputs.package-name }} --names-to-skip=${{ inputs.names-to-skip }}
shell: bash
11 changes: 0 additions & 11 deletions .github/workflows/call_check_docstrings.yaml

This file was deleted.

52 changes: 0 additions & 52 deletions .github/workflows/check_docstrings.yaml

This file was deleted.

17 changes: 17 additions & 0 deletions .github/workflows/test_check_docstrings.yaml

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

pauladkisson marked this conversation as resolved.
Show resolved Hide resolved
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
name: Call Check Docstrings
on:
workflow_dispatch:

jobs:
check-docstrings:
runs-on: ubuntu-latest
name: Check Docstrings
steps:
- id: check_docstrings
uses: catalystneuro/.github/.github/actions/check_docstrings@main
pauladkisson marked this conversation as resolved.
Show resolved Hide resolved
with:
python-version: '3.10'
repository: 'catalystneuro/roiextractors'
branch: 'main'
package-name: 'roiextractors'
names-to-skip: 'skipped_fn'
5 changes: 4 additions & 1 deletion tests/conftest.py
Original file line number Diff line number Diff line change
Expand Up @@ -4,10 +4,13 @@

def pytest_addoption(parser):
parser.addoption("--package", action="store")
parser.addoption("--names-to-skip", action="store", default="")

def pytest_generate_tests(metafunc):
pauladkisson marked this conversation as resolved.
Show resolved Hide resolved
package_name = metafunc.config.getoption("--package")
names_to_skip = metafunc.config.getoption("--names-to-skip")
names_to_skip = names_to_skip.split(",") if names_to_skip else []
pauladkisson marked this conversation as resolved.
Show resolved Hide resolved
package = import_module(package_name)
objs = traverse_package(package, package_name)
objs = traverse_package(package, package_name, names_to_skip=names_to_skip)
if "obj" in metafunc.fixturenames:
metafunc.parametrize("obj", objs)
6 changes: 3 additions & 3 deletions tests/traverse.py
Original file line number Diff line number Diff line change
Expand Up @@ -55,7 +55,7 @@ def traverse_module(module: ModuleType, parent: str, names_to_skip: Optional[Lis

if isinstance(object_, type) and object_.__module__.startswith(parent): # class
class_object = object_
class_functions = traverse_class(class_object=class_object, parent=parent)
class_functions = traverse_class(class_object=class_object, parent=parent, names_to_skip=names_to_skip)
local_classes_and_functions.append(class_object)
local_classes_and_functions.extend(class_functions)

Expand Down Expand Up @@ -97,13 +97,13 @@ def traverse_package(package: ModuleType, parent: str, names_to_skip: Optional[L
and object_.__package__.startswith(parent)
):
subpackage = object_
subpackage_members = traverse_package(package=subpackage, parent=parent)
subpackage_members = traverse_package(package=subpackage, parent=parent, names_to_skip=names_to_skip)
local_packages_and_modules.append(subpackage)
local_packages_and_modules.extend(subpackage_members)

elif isinstance(object_, ModuleType) and object_.__package__.startswith(parent):
module = object_
module_members = traverse_module(module=module, parent=parent)
module_members = traverse_module(module=module, parent=parent, names_to_skip=names_to_skip)
local_packages_and_modules.append(module)
local_packages_and_modules.extend(module_members)

Expand Down