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

Core: Fix auto-fill in the text client when clicking on a hint suggestion #3267

Open
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

Ishigh1
Copy link
Contributor

@Ishigh1 Ishigh1 commented May 4, 2024

What is this fixing or adding?

#2833 added more details to failed hints commands, which broke the hint suggestion copy system that was expecting a different string, so I fixed it here

How was this tested?

By left clicking the different things that are auto-fillable when clicked on

If this makes graphical changes, please attach screenshots.

image
(previous behavior was the same but nothing in the textbox after a click)

@github-actions github-actions bot added affects: core Issues/PRs that touch core and may need additional validation. waiting-on: peer-review Issue/PR has not been reviewed by enough people yet. labels May 4, 2024
@Exempt-Medic Exempt-Medic added the is: bug/fix Issues that are reporting bugs or pull requests that are fixing bugs. label May 4, 2024
Copy link
Collaborator

@Exempt-Medic Exempt-Medic left a comment

Choose a reason for hiding this comment

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

Code LGTM. Tested it locally and it worked, even with some pathological-seeming item names like Watch '? ( Battery because of get_text_between using rindex

@Ishigh1
Copy link
Contributor Author

Ishigh1 commented May 4, 2024

Now that you say it, there's an edge case that won't work if the player hints for something that contains did you mean ' or ends with did you mean
This case is so extreme I doubt it would be useful to spend time fixing it (the result is just something like !hint ', did you mean 'Hazardous Mechs), and there would be another edge case that would probably appear anyway, unless we totally change the way it works and the server sends a suggestion separately

@Berserker66
Copy link
Member

Bonus Points for a Unittest that prevents this in the future.

MultiServer.py Outdated Show resolved Hide resolved
@beauxq
Copy link
Collaborator

beauxq commented May 17, 2024

I think making a new module util alongside Utils should not be part of this bug-fix pull request. (And I think other people expressed this in the discord discussions about it.)

I think it would be better to put it in Utils.py here, and another pull request could be made for reorganizing modules.

@Ishigh1
Copy link
Contributor Author

Ishigh1 commented May 17, 2024

Well then, where in Utils? At the end? No one gave me clear indications except Black Silver who said to put it inside a util module

@beauxq
Copy link
Collaborator

beauxq commented May 17, 2024

Well then, where in Utils?

after get_fuzzy_results

@Ishigh1
Copy link
Contributor Author

Ishigh1 commented May 17, 2024

Well I moved it to where you suggested

Utils.py Outdated Show resolved Hide resolved
Utils.py Outdated Show resolved Hide resolved
@Ishigh1
Copy link
Contributor Author

Ishigh1 commented May 17, 2024

Right, I forgot to reformat the file for once

Utils.py Outdated Show resolved Hide resolved
Copy link
Collaborator

@beauxq beauxq left a comment

Choose a reason for hiding this comment

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

tested without looking too hard for edge cases

looked over code

@Exempt-Medic Exempt-Medic added waiting-on: core-review Issue/PR has been peer-reviewed and is ready to be merged or needs input from a core maintainer. and removed waiting-on: peer-review Issue/PR has not been reviewed by enough people yet. labels May 20, 2024
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. waiting-on: core-review Issue/PR has been peer-reviewed and is ready to be merged or needs input from a core maintainer.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants