Skip to content

Allow priority override#48

Merged
emilyyuan03 merged 7 commits intomainfrom
priority-override
Jan 28, 2026
Merged

Allow priority override#48
emilyyuan03 merged 7 commits intomainfrom
priority-override

Conversation

@emilyyuan03
Copy link
Copy Markdown
Contributor

No description provided.

@wakingrufus
Copy link
Copy Markdown
Member

Looks good so far, just 1 comment


fun priority(priority: String) {
try {
this.priority = Priority.valueOf(priority)
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I think we can avoid throwing an exception here. instead, get the set of valid values, check if the arg is in that set, if not, lets log a warning, and just not set the priority

var priority = result.getPriority();
String ruleClassName = ruleClass.getClass().getCanonicalName();
for (Map.Entry<String, Priority> override : overrides.entrySet()) {
if (ruleClassName.startsWith(override.getKey())) {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

i don't think we should do this based on class name, since rules within the same class often have different priorities. I think it should be based on rule name, and use equalsIgnoreCase to compare... WDYT?


fun priority(priority: String) {
try {
this.priority = Priority.valueOf(priority)
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This still needs refactoring to avoid the exception

wakingrufus
wakingrufus previously approved these changes Jan 28, 2026
@wakingrufus
Copy link
Copy Markdown
Member

Lgtm! Can you add some docs to the readme for this feature? Thanks!

@emilyyuan03 emilyyuan03 merged commit bd5b869 into main Jan 28, 2026
3 checks passed
@emilyyuan03 emilyyuan03 deleted the priority-override branch January 28, 2026 21:32
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.

3 participants