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

Framework for multi-search. #3493

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

Conversation

jaborsh
Copy link
Contributor

@jaborsh jaborsh commented Apr 7, 2024

Brief overview of PR changes/additions

This PR isn't intended to go live yet (although it won't break anything). This is my proposal on how we rebuild the search function in order to handle queries such as: get sword, get 2 swords, and get all as discussed in #3468.

With this framework, I use regular expression patterns within parse for functions get, drop, give, and put which would also need to be implemented, but I think this gives an idea for a direction to go. If desired, I can attach CmdGet and CmdDrop.

@jaborsh
Copy link
Contributor Author

jaborsh commented Apr 7, 2024

There's also a series of linter formatting. I can undo that if desired.

@@ -530,13 +539,17 @@ def handle_search_results(self, searchdata, results, **kwargs):

nofound_string = kwargs.get("nofound_string")
multimatch_string = kwargs.get("multimatch_string")
return_quantity = kwargs.get("return_quantity")
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Point of interest.

@@ -2354,48 +2413,65 @@ def display_len(target):
# Replace this hook function by changing settings.SEARCH_AT_RESULT.


class SearchReturnType(Enum):
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Point of interest.

@jaborsh jaborsh marked this pull request as draft April 7, 2024 18:46
Copy link
Member

@Griatch Griatch left a comment

Choose a reason for hiding this comment

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

I need to look into this a bit more, have a few questions so far. This also doesn't actually exemplifies how this would be used which could be the next step.

Also, please use the correct line width in your IDE (Evennia uses 100 character line width) to make your PR diff smaller (and you won't have to specify your 'points of interests'). As it is I'll have to reformat your code after merging.

Comment on lines +76 to +80
if any(
sessid
for sessid in self._sessid_cache
if sessid not in evennia.SESSION_HANDLER
):
Copy link
Member

Choose a reason for hiding this comment

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

Please set up your IDE to follow Evennia line width standard (100 characters). That makes the PR smaller (easier to review) and clearer since you only see the important changes.

matches = None
elif len(matches) > 1:
return_quantity = kwargs.get("return_quantity", 1)
return_type = kwargs.get("return_type", SearchReturnType.DEFAULT)
if return_type == SearchReturnType.ONE:
Copy link
Member

Choose a reason for hiding this comment

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

This can be a match ... case structure.

return_quantity = kwargs.get("return_quantity", 1)
return_type = kwargs.get("return_type", SearchReturnType.DEFAULT)
if return_type == SearchReturnType.ONE:
return matches[return_quantity - 1]
Copy link
Member

Choose a reason for hiding this comment

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

Would this not traceback if matches and return_quantity are out of sync?

elif return_type == SearchReturnType.MULTIPLE:
matches = matches[:return_quantity]
return matches
elif return_type == SearchReturnType.ALL:
Copy link
Member

Choose a reason for hiding this comment

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

Is this not the default?

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

2 participants