-
Notifications
You must be signed in to change notification settings - Fork 1k
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
Combines legacy qv-pattern and advanced filter pattern in history index endpoint #17368
Conversation
eede752
to
1a4e129
Compare
26fbc2b
to
a3d7365
Compare
a3d7365
to
482fae5
Compare
if search is None: | ||
return self.service.index( | ||
trans, serialization_params, filter_query_params, deleted_only=deleted, all_histories=all | ||
) | ||
else: | ||
payload = HistoryIndexQueryPayload.construct( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't love this, I would have preferred an "adapter" step to try to convert those new query parameters to fit in the filter_query_params
instead of the complete branching. Although I haven't checked if there is any limitation on this.
Anyway, I guess is better than having the extra route for now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree with you, this is not very pretty. I think we have to decide wether to combine the two code paths or deprecate the legacy path moving forward. I would favor the latter, since this would also streamline the client request, which in the history panel, converts the modern search string to q/qv values as of right now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we go ahead with deprecating q/qv, it would be nice if the new syntax still supported queries that we currently expose in BioBlend, e.g.:
https://github.com/galaxyproject/bioblend/blob/main/bioblend/galaxy/histories/__init__.py#L135
Not saying that it doesn't, I just wanted to flag it.
Then BioBlend can transparently use one syntax or the other, depending on the Galaxy version.
81d87b2
to
482fae5
Compare
482fae5
to
1cf5d8b
Compare
Requires #17282. This PR combines the legacy q/qv and the advanced filter history index endpoints as discussed in the backend group meeting. In a follow-up the q/qv endpoint parameters will be deprecated.
How to test the changes?
(Select all options that apply)
License