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

Soften wording for rule to avoid using default in switch statements #245

Merged
merged 4 commits into from
Nov 10, 2023

Conversation

calda
Copy link
Member

@calda calda commented Oct 18, 2023

Summary

We have a rule that suggests to avoid using the default case when switching over an enum.

This rule is worded pretty strongly ("never use the default case"), but lacks autocorrect or linting, so has no enforcement. I also think there are valid reasons to use the default case.

I think we should just soften the wording of this rule (not use strict wording like "never"), since it is still good advice overall.

Some alternatives we could consider:

  1. Add a lint rule to enforce this rule
    • This is too heavy-handed, because there are valid use cases for using default. The prose rule includes a set of exceptions where it can make sense to use default.
    • This is infeasible, because a linter wouldn't know if we are switching over an enum or not. We may be switching over something that can't be switched over exhaustively, which would require a default case. (edit: Michael has suggested some heuristics we could use to reduce the number of false positives here without needing full type information).
  2. Remove this rule completely
    • We don't have many rules that are mere suggestions, without any linter enforcement or autocorrection. We haven't added any new rules like this in many years.
    • I think we should continue to permit this sorts of rule, which makes recommendations that can result in higher-quality code but aren't strictly enforced. This is probably the biggest "expansion opportunity" for the content in our style guide.

Reasoning

Please react with 👍/👎 if you agree or disagree with this proposal.

@bachand
Copy link
Contributor

bachand commented Oct 18, 2023

My opinion is that we should keep this rule worded as-is.

In a large codebase, it can be very hard (I would argue infeasible) to validate all switch statements over an enumeration when adding a new case. This style guide rule makes that intractable validation problem trivial to validate, since the compiler does the work for us. IMHO the huge time savings and increase in confidence that this rule brings justifies the lack of enforcement and autocorrection. Our style guide indicates that exceptions should be heavily justified.

Screenshot 2023-10-18 at 12 07 13 PM

I think this is one of the cases where the benefits of an unenforceable rule outweigh the exceptionality of it.

README.md Outdated Show resolved Hide resolved
@calda
Copy link
Member Author

calda commented Nov 6, 2023

Worked with @bachand to workshop and simplify the changes

@bachand
Copy link
Contributor

bachand commented Nov 7, 2023

My opinion is that we should keep this rule worded as-is.

In a large codebase, it can be very hard (I would argue infeasible) to validate all switch statements over an enumeration when adding a new case. This style guide rule makes that intractable validation problem trivial to validate, since the compiler does the work for us. IMHO the huge time savings and increase in confidence that this rule brings justifies the lack of enforcement and autocorrection. Our style guide indicates that exceptions should be heavily justified.

Screenshot 2023-10-18 at 12 07 13 PM I think this is one of the cases where the benefits of an unenforceable rule outweigh the exceptionality of it.

Worked with @bachand to workshop and simplify the changes

Thanks for taking the time to work through this one together @calda . The large number of places in our codebase that are in conflict with this rule as well as the reasonable exceptions that we discussed (partially documented in the new examples) convinced that we should in fact soften the wording.

Copy link
Contributor

@bachand bachand left a comment

Choose a reason for hiding this comment

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

🎉

README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
Co-authored-by: Michael Bachand <michael.bachand@airbnb.com>
@calda calda merged commit 3781dbd into master Nov 10, 2023
5 checks passed
@calda calda deleted the cal--switch-default branch November 10, 2023 16: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