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

Rework Object searching to behave more consistently #3541

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

Conversation

InspectorCaracal
Copy link
Contributor

@InspectorCaracal InspectorCaracal commented May 15, 2024

Brief overview of PR changes/additions

This PR makes modifications to both the DefaultObject manager methods of get_objs_by_key_and_alias and search_object.

get_objs_by_key_and_alias will now properly respect partial matches in its queries, leading to the expected result of an empty search string partial-matching all candidates and additionally running fewer queries.

search_object has been moderately streamlined, removing the need for special-case handling of tags or search terms, and applying all filtering keywords consistently to all types of searches rather than unintuitively picking and choosing based on which combination of keywords were provided.

Motivation for adding to Evennia

Usability improvements

Other info (issues closed, discussion etc)

The undesirable behavior brought up on Discord where searching with tag criteria but no search string is resolved by this PR. It additionally resolves several equally problematic behaviors that currently result when giving the search method an empty search string, such as returning an empty result when people typically expect an empty string to partial-match all keys not none, and the unfortunate tag-only search disregarding all other keywords entirely.

@InspectorCaracal InspectorCaracal marked this pull request as draft May 15, 2024 18:46
@InspectorCaracal
Copy link
Contributor Author

InspectorCaracal commented May 15, 2024

Whoops.

Converting this to a draft because a) there's more failing unit tests, and b) I just realized while working on adding new tests for the two methods I modified that I unwittingly lost the match scoring mechanism. Since that needs to be accounted for, I need to rethink some of my changes to get_objs_with_key_or_alias before this can be merged.

@InspectorCaracal
Copy link
Contributor Author

InspectorCaracal commented May 15, 2024

On further review of how string_partial_matching works, it seems that the "score" is just how many of the input words were matched in the alternative text. Since the score is zeroed when any input words are lacking, this means my single query will in practice return equivalent results since it requires that all input words are partially matched in order. As such, I'm leaving the modifications as I originally wrote them.

The failed unit tests are a bit more of a pain, however. It seems as though the clothing contrib, rather than having a proper usage feedback on the remove command when used with no args, was set up to verify that it returned a "no object found" error instead. Since caller.search is fuzzy by default, this is obviously failing now that the empty term fuzzy search works "correctly"....

@@ -85,7 +85,10 @@ def test_clothingcommands(self):
)

# Test remove command.
self.call(clothing.CmdRemove(), "", "Could not find ''.", caller=self.wearer)
# NOTE: commenting out due to failing via the search refactor - however, this command
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've temporarily just commented out this assertion since I intend to make a second PR to correct the oversight in the contrib command, but I'm unsure if this is the correct approach.

I could alternatively write the other PR, wait for it to be merged, and rebase this one onto it.

Or, I could leave this one failing with the assumption I'll correct it. (I'm not a fan of this third option. >.>)

@InspectorCaracal InspectorCaracal marked this pull request as ready for review May 15, 2024 23:44
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.

None yet

1 participant