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

Rule: default-over-else #360

Merged
merged 1 commit into from
Oct 2, 2023
Merged

Rule: default-over-else #360

merged 1 commit into from
Oct 2, 2023

Conversation

anderseknert
Copy link
Member

@anderseknert anderseknert commented Oct 1, 2023

Prefer default assignment over catch-all else. This rule was originally meant to endorse the new default function construct, but having tested it out thoroughly for this rule, I'm now more inclined to have it flagged. I find that counter-intuitive, and as such more likely to cause harm than good. Those who disagree may configure the prefer-default-functions option. Either way, default assignment over the equivalent else feels more idiomatic to me, so the rule feels relevant despite not doing what was originally intended.

Fixes #211

Prefer default assignment over catch-all `else`. This rule
was originally meant to endorse the new default function construct,
but having tested it out thoroughly for this rule, I'm now more
inclined to have it flagged. I find that counter-intuitive, and
as such more likely to cause harm than good. Those who disagree
may configure the `prefer-default-functions` option. Either way,
default assignment over the equivalent `else` feels more idiomatic
to me, so the rule feels relevant despite not doing what originally
intended.

Fixes #211

Signed-off-by: Anders Eknert <anders@styra.com>
Copy link
Member

@charlieegan3 charlieegan3 left a comment

Choose a reason for hiding this comment

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

LGTM.

I had the same assumption about undefined args to functions 🤔

@anderseknert
Copy link
Member Author

Thanks @charlieegan3! Yeah, I feel like the OPA docs should be amended to mention that, as using default functions without being aware seems like it could lead to some unintended things happening. I'll create an issue for that in the OPA backlog.

@anderseknert anderseknert merged commit 52c57d3 into main Oct 2, 2023
1 check passed
@anderseknert anderseknert deleted the default-over-else branch October 2, 2023 08:58
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.

Prefer default function to conditionless else
2 participants