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

Removing incorrect comment about a warning #1132

Merged
merged 1 commit into from
Oct 7, 2024

Conversation

madolson
Copy link
Member

@madolson madolson commented Oct 6, 2024

There is a lot of bad legacy usage of default: with enums, which is an anti-pattern. If you omit the default, the compiler will tell you if a new enum value was added and that it is missing from a switch statement.

Someone mentioned on another PR they used default: because of this warning, so just removing it, but might create an issue to do a wider cleanup.

Signed-off-by: Madelyn Olson <madelyneolson@gmail.com>
Copy link

codecov bot commented Oct 6, 2024

Codecov Report

Attention: Patch coverage is 0% with 1 line in your changes missing coverage. Please review.

Project coverage is 70.62%. Comparing base (b77440a) to head (6653efe).
Report is 13 commits behind head on unstable.

Files with missing lines Patch % Lines
src/cluster_legacy.c 0.00% 1 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff              @@
##           unstable    #1132      +/-   ##
============================================
- Coverage     70.63%   70.62%   -0.02%     
============================================
  Files           114      114              
  Lines         61710    61716       +6     
============================================
- Hits          43586    43584       -2     
- Misses        18124    18132       +8     
Files with missing lines Coverage Δ
src/cluster_legacy.c 86.15% <0.00%> (-0.01%) ⬇️

... and 21 files with indirect coverage changes

Copy link
Contributor

@zuiderkwast zuiderkwast left a comment

Choose a reason for hiding this comment

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

It was added in light publish iirc. I wasn't sure why there would be a warning without it, but i didn't look into it.

Copy link
Member

@PingXie PingXie left a comment

Choose a reason for hiding this comment

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

Agreed that this was an anti-pattern and we should switch to serverPanic().

@madolson madolson merged commit e617bf2 into valkey-io:unstable Oct 7, 2024
47 checks passed
naglera pushed a commit to naglera/placeholderkv that referenced this pull request Oct 10, 2024
There is a lot of bad legacy usage of `default:` with enums, which is an
anti-pattern. If you omit the default, the compiler will tell you if a
new enum value was added and that it is missing from a switch statement.

Someone mentioned on another PR they used `default:` because of this
warning, so just removing it, but might create an issue to do a wider
cleanup.

Signed-off-by: Madelyn Olson <madelyneolson@gmail.com>
Signed-off-by: naglera <anagler123@gmail.com>
eifrah-aws pushed a commit to eifrah-aws/valkey that referenced this pull request Oct 20, 2024
There is a lot of bad legacy usage of `default:` with enums, which is an
anti-pattern. If you omit the default, the compiler will tell you if a
new enum value was added and that it is missing from a switch statement.

Someone mentioned on another PR they used `default:` because of this
warning, so just removing it, but might create an issue to do a wider
cleanup.

Signed-off-by: Madelyn Olson <madelyneolson@gmail.com>
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.

4 participants