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

MultiServer: Make it so hint_location doesn't set an automatic priority #4713

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

NewSoupVi
Copy link
Member

@NewSoupVi NewSoupVi commented Mar 9, 2025

Alternative standalone implementation of #4674

I also decided I didn't like how the current code or the code in that PR tied automatic behavior to specific passed statuses. In my opinion, if one specific status is explicitly passed to these functions, that status should be used exactly (except in the case of "found").

As such, there are two scenarios:

  1. You want an automatic status based on item quality
  2. You have a specific status in mind
    2.a This "specific status" happens to be HintStatus.UNSPECIFIED

I think it is cleaner and more elegant to express this as status: HintStatus | None, where "None" means "make an automatic status", so that's how I ended up implementing this.

Tested:
Lightly

@NewSoupVi NewSoupVi added is: bug/fix Issues that are reporting bugs or pull requests that are fixing bugs. is: refactor/cleanup Improvements to code/output readability or organizization. affects: core Issues/PRs that touch core and may need additional validation. labels Mar 9, 2025
@github-actions github-actions bot added the waiting-on: peer-review Issue/PR has not been reviewed by enough people yet. label Mar 9, 2025
@NewSoupVi
Copy link
Member Author

NewSoupVi commented Mar 9, 2025

I did also notice how every instance of collect_hints currently uses automatic priority, and every instance of collect_hint_location_id / collect_hint_location_name uses "Unspecified" priority, which means in theory the code could be further simplified to just hard code each behavior to each function.

However, I do actually have plans for setting specific priorities even in case of a location-derived hint, so I decided to keep the status arg for these functions, and just default them to None and Unspecified respectively

Copy link
Contributor

@EmilyV99 EmilyV99 left a comment

Choose a reason for hiding this comment

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

Looks great! Read through all the changes, though didn't test myself.

Hope this gets merged ASAP (in time for 0.6.0!)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
affects: core Issues/PRs that touch core and may need additional validation. is: bug/fix Issues that are reporting bugs or pull requests that are fixing bugs. is: refactor/cleanup Improvements to code/output readability or organizization. waiting-on: peer-review Issue/PR has not been reviewed by enough people yet.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants