Skip to content

Improve docstings of OpenEphysBinaryRawIO - #1774

Merged
alejoe91 merged 6 commits into
NeuralEnsemble:masterfrom
h-mayorquin:improve_open_ephys_docs
Oct 6, 2025
Merged

alejoe91 merged 6 commits into
NeuralEnsemble:masterfrom
h-mayorquin:improve_open_ephys_docs

Conversation

@h-mayorquin

@h-mayorquin h-mayorquin commented Sep 12, 2025 •

Copy link
Copy Markdown
Contributor

While working on fixing a neuroconv issue, I noticed that the OpenEphysBinaryRawIO class lacks clear documentation about how the folder structure of the Open Ephys binary format is parsed and use internally.

This PR improves the situation in two ways:

  1. The docstring now more explicitly describes the type of structure being parsed and clarifies that dirname can point to different levels within the folder tree, each yielding different parsing results.

  2. The current explore_folder function mixes two responsibilities:

    • Parsing the folder structure.
    • Mapping that structure into Neo.

    To separate concerns, I split this logic into two private functions. Each now has a dedicated docstring that clearly explains its role. There is no change in implementation the function is just split in two.

@h-mayorquin
h-mayorquin marked this pull request as ready for review September 12, 2025 22:45
@zm711

zm711 commented Sep 14, 2025

Copy link
Copy Markdown
Contributor

I think this looks pretty great. The diff is huge since the functions are split so it'll take a moment to review :)

@zm711 zm711 added this to the 0.14.3 milestone Sep 19, 2025
@alejoe91
alejoe91 self-requested a review September 19, 2025 13:29

@alejoe91 alejoe91 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Nice rewrite @h-mayorquin! This added docs will make it much easier to maintain for us too! :)

@alejoe91
alejoe91 merged commit c7c90f6 into NeuralEnsemble:master Oct 6, 2025
3 of 5 checks passed
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