-
Notifications
You must be signed in to change notification settings - Fork 188
Remove WAVEFORM_SUPPORT feature toggle #886
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 WAVEFORM_SUPPORT feature toggle #886
Conversation
Test Results 42 files ± 0 42 suites ±0 1h 3m 38s ⏱️ -37s Results for commit 97fd445. ± Comparison against base commit b97ffcd. This pull request removes 24 tests. |
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.
It's too early for this. Read https://dev.azure.com/ni/DevCentral/_wiki/wikis/AppCentral.wiki/900/Feature-Toggles
Feature toggles should only be removed once the code protected behind the feature toggle ships in the enabled state. Do not remove a feature toggle before the code ships.
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.
Ah, I didn't know that policy was the same for GitHub repos, but it makes sense. I'll abandon this PR for now. Also, I'll change the work item from "remove the feature toggle" to "enable the feature toggle by default".
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 don't want to break compatibility after release. If AB#3178052 is not fixed by release, then I think we should disable the waveform feature toggle.
# Note, the data on the port's waveform is MSB instead of LSB because of bug AB#3178052
# When that bug is fixed, these asserts should be updated
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've added AB#3178052 as a prerequisite for the "enable the waveform feature toggle" work item AB#3367327.
| * Completed support for reading and writing Waveforms, and removed the WAVEFORM_SUPPORT feature toggle. | ||
| * Added support for reading and writing Waveform data through gRPC using [NI gRPC Device Server](https://github.com/ni/grpc-device). |
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.
The parent bullet is unnecessary and only matters to developers, not users.
|
We can't remove this feature toggle until after release. |
I've added tests applicable for this pull requestWhat does this Pull Request accomplish?
Now that all the read waveform and write waveform methods have full support for grpc-device, we can remove the WAVEFORM_SUPPORT feature toggle.
Why should this Pull Request be merged?
AB#3424643
What testing has been done?
Ran all the auto tests
Ran all the affected input examples