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

Improve documentation for MutliDeviceFactories #276

Merged
merged 2 commits into from
Sep 5, 2024
Merged

Improve documentation for MutliDeviceFactories #276

merged 2 commits into from
Sep 5, 2024

Conversation

jonnew
Copy link
Member

@jonnew jonnew commented Aug 30, 2024

@jonnew jonnew requested a review from cjsha August 30, 2024 22:06
@cjsha
Copy link
Member

cjsha commented Sep 4, 2024

In each of the hubs, in the top-level remarks, you added these nice bulleted list of devices and their corresponding features. It might help to refer to your bulleted list as a list of devices in the device group because the summary for MultiDeviceFamily's Process method (which is going to be the next thing in the page after I change the template according to previous conversations), it says "Configure all devices in the device group". This implicitly defines what a device group is.

@cjsha
Copy link
Member

cjsha commented Sep 4, 2024

I like what you're calling the headstages, they're following a format: Headstage-Abc (i.e. Headstage-NeuropixelsV2e). I think there's practicality in this uniformity. Though I'm not sure we need hyphens when it comes to human-readable content that goes in docs and store. And I think Headstage-NeuropixelsV2e-Beta should be Headstage-NeuropixelsV2eBeta instead

Copy link
Member

@cjsha cjsha left a comment

Choose a reason for hiding this comment

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

This PR is approved but please go through and consider all the comments - I'm not sure it requires another review cycle after you address them (though you can re-request me if you want)

OpenEphys.Onix1/ConfigureBreakoutBoard.cs Outdated Show resolved Hide resolved
OpenEphys.Onix1/ConfigureBreakoutBoard.cs Outdated Show resolved Hide resolved
OpenEphys.Onix1/ConfigureHeadstage64.cs Outdated Show resolved Hide resolved
OpenEphys.Onix1/ConfigureNeuropixelsV1eHeadstage.cs Outdated Show resolved Hide resolved
OpenEphys.Onix1/ConfigureNeuropixelsV2eBetaHeadstage.cs Outdated Show resolved Hide resolved
OpenEphys.Onix1/ConfigureNeuropixelsV2eHeadstage.cs Outdated Show resolved Hide resolved
- Add features to class description in bullet list format.
@jonnew jonnew merged commit aa5244d into main Sep 5, 2024
7 checks passed
@jonnew jonnew deleted the issue-273 branch September 5, 2024 16:49
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.

ConfigureHeadstage64 constructor XML remarks should be moved to class description
2 participants