-
Notifications
You must be signed in to change notification settings - Fork 3
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
Conversation
There was a problem hiding this 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.
It still builds correctly, shows the correct output types, and does not appear to throw any errors.
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? |
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 |
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 |
- 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
This commit requires a major revision incrementBecause we are in the 0 major revision, this change is permitted, even though its API breakingBreakout
from names of static hardware definition classes withinConfigure<>
classes #297BreakoutAnalogInput
toAnalogInput
#300