-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
Allow suffixes for filter runs #5252
Allow suffixes for filter runs #5252
Conversation
Update to latest uptream
sync upstream
An open question is whether we would want to support multiple suffixes? |
I think that would be consistent with the existing (newer) operator suffix syntax. |
@Jermolene I thought as much and it doesn't hurt to have the support there even if we don't necessarily use it right away. I will re-work this PR to support multiple suffixes post 5.1.23 |
One core named prefix, But that's not a reason to squeeze this into 5.1.23. I agree with delaying it until 5.1.24. |
@Jermolene I have extended this to accommodate multiple filter run prefix suffixes as previously discussed. As a recap, after some discussion we had agreed to implement suffixes for filter runs.
|
With regards to the alternate proposal for a syntax like Compare the following (please disregard the logic of the filter, its just an example of a longer filter run):
With the latter, it is immediately understood how the filter run will be applied, whereas with the former you don't realize this until after reading the entire run. While the former syntax might seem familiar to the developers amongst us as it resembles passing arguments to a function, I think for most of our users the latter will be far easier to understand. As for being able to use variables as suffixes, currently our parser doesn't support either of the above two syntax. So support for variables isn't a given in either one, and could be implemented for either if so desired. Also, I think it would be acceptable for each way of filtering to have its own strengths. For example comparing the |
Sounds good thanks @saqimtiaz, I'll have a look at the code. |
Fantastic, thanks @saqimtiaz |
Allows for suffixes to filter run prefixes, as discussed in #5166
Also see here
@Jermolene Suggest we delay this for 5.1.24 since no core prefixes use this at the moment, and thus we can have more time to make a considered decision and test more thoroughly. Just pushing this PR now so I do not forget.