Skip to content

Conversation

@not-napoleon
Copy link
Member

Just a quick refactor over Friday ☕

I came across this while reviewing a PR the other day. It's a bit of a code smell to have a switch on an enum like this, so I've replaced it with a required enum parameter. This makes it clear to anyone adding to the enum that this is a required field. While I was at it, I also removed the other switch, since it was redundant with the name field anyway.

@elasticsearchmachine
Copy link
Collaborator

Pinging @elastic/es-storage-engine (Team:StorageEngine)

Copy link
Member

@martijnvg martijnvg left a comment

Choose a reason for hiding this comment

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

☕ LGTM

Copy link
Contributor

@jordan-powers jordan-powers left a comment

Choose a reason for hiding this comment

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

LGTM, always nice to see a PR with a net negative LOC!

@not-napoleon not-napoleon merged commit d557d5c into elastic:main Sep 2, 2025
33 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants