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

Remove Breakout prefix from classes where its redundant #301

Merged
merged 2 commits into from
Sep 17, 2024
Merged

Conversation

jonnew
Copy link
Member

@jonnew jonnew commented Sep 16, 2024

Copy link
Member

@bparks13 bparks13 left a comment

Choose a reason for hiding this comment

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

I see no issues with this PR. There are some missing XML comments from the newly created obsolete classes, but those are easy to fix.

Visually, if someone opens up an existing workflow that has one of these obsolete nodes, it shows the correct color but appears with cross-hatches to indicate that something is up.

image

It still builds correctly, shows the correct output types, and does not appear to throw any errors.

OpenEphys.Onix1/AnalogInput.cs Show resolved Hide resolved
OpenEphys.Onix1/AnalogOutput.cs Show resolved Hide resolved
OpenEphys.Onix1/ConfigureAnalogIO.cs Show resolved Hide resolved
OpenEphys.Onix1/ConfigureAnalogIO.cs Outdated Show resolved Hide resolved
OpenEphys.Onix1/ConfigureBreakoutBoard.cs Show resolved Hide resolved
OpenEphys.Onix1/ConfigureDigitalIO.cs Show resolved Hide resolved
OpenEphys.Onix1/DigitalInput.cs Show resolved Hide resolved
OpenEphys.Onix1/DigitalOutput.cs Show resolved Hide resolved
@jonnew
Copy link
Member Author

jonnew commented Sep 17, 2024

Thanks for the review. The big issue we need to consider is do we put effort into maintaining backwards compatibility when we are in version 0.X.X? Im in the middle of refactoring the Bno055 to create a common class for all headstages that perform polling. Should I add these obsolete derived classes there as well? I guess so?

@bparks13
Copy link
Member

I guess the question for BNO is what is the opportunity cost for adding the obsolete derived classes? I'm guessing there aren't many workflows that depend on this yet, but I think it would be good to add the derived classes if it is easy to do, and then remove the obsolete classes once we reach 1.0

@jonnew
Copy link
Member Author

jonnew commented Sep 17, 2024

That's precisely what I was thinking as well. I will add them to the refactored Bno stuff. I also realized you can put a note in the [Obsolete] attribute. We will state there they will be removed in 1.0.0

- Preserve backwards compatibility with old workflows.
- This commit requires a major revision increment
- Add missing XML comments to public, obsolete types
- Minor changes in xml comments
@jonnew jonnew merged commit b787415 into main Sep 17, 2024
7 checks passed
@jonnew jonnew deleted the issue-297 branch September 17, 2024 21:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants