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

[IDEA] :sort prefix and extensible filter run prefixes #5166

Closed
saqimtiaz opened this issue Dec 1, 2020 · 21 comments · Fixed by #5653
Closed

[IDEA] :sort prefix and extensible filter run prefixes #5166

saqimtiaz opened this issue Dec 1, 2020 · 21 comments · Fixed by #5653

Comments

@saqimtiaz
Copy link
Member

Just as the :filter filter run prefix provides an inline alternative to the filter operator, it would be desirable to have a :sort filter run prefix where the list is sorted by comparing the results of applying the filter run to each list item. This would be an inline alternative to the sortsub operator.

[tag[HelloThere]] :sort[get[text]length[]]

instead of:

<$vars compare-by-title-length="[length[]]">
[tag[HelloThere]sortsub:number<compare-by-title-length>]
</$vars>

However, we currently support multiple types of sorting: string, integer, number, date and version

So either we would need to add 5 filter run prefixes, one for each of the sort types. :sortstring, :sortinteger etc...

Or we could consider supporting suffixes for filter runs. Example : [tag[HelloThere]] :sort:string[........] or [tag[HelloThere]] :sort:number[........]
While this second approach is a lot more flexible and powerful, the syntax might prove not so intuitive for new users.

Is there a third alternative that I am not thinking of?

I am interested in implementing this - even if only as a plugin - so I would appreciate any feedback or ideas.

@rmunn
Copy link
Contributor

rmunn commented Dec 1, 2020

Not only that, there's also sortan for alphanumeric sorting, there's reverse sorting (high to low instead of low to high)... it could get very complicated very fast.

I strongly dislike the idea of having suffixes for filter run prefixes (:sort:number and so on). Definite comprehension issues for new users. If there's enough benefit to add a :sort prefix, then it's worth adding all of the various sorting variants as their own prefix (:sortn, :sortan, :revsort and so on). But I don't think there is enough benefit. I think <$vars sort-by="[some-filter[]]"> followed by sortsub<some-filter> is simple enough that the prefix version doesn't gain all that much, and when you take into account the dozen different sorting prefixes we would need to account for all possibilities, I think the complexity cost is more than what we'd gain.

@saqimtiaz
Copy link
Member Author

@rmunn the feeling is mutual about suffixes for filter runs and comprehension. The sort options I listed are the ones supported by the sortsub operator rather than the sort operator.

As for the utility of the sort prefix, I find filters written inline far easier to read, comprehend and maintain.

when you take into account the dozen different sorting prefixes we would need to account for all possibilities, I think the complexity cost is more than what we'd gain.

Yes that is the issue and that is why this is an issue for discussion rather than a PR. I realize this might not be appropriate for the core and am happy writing this as a plugin for myself. The key advantage gained with the refactoring to support the filterrunprefix modules is that plugins can now add filter run prefixes.

However before I started writing code, I wanted to talk this out and see if there was a third alternative I was missing as neither proposed solution is entirely satisfactory.

@rmunn
Copy link
Contributor

rmunn commented Dec 1, 2020

So are you going to end up creating a prefix version of every filter that takes a subfilter argument, like reduce, so that it can be written inline?

As for a third alternative, I'm not seeing one. I think you'll just have to implement a separate filter prefix for every possible variant, finding appropriate names. (Though implementation can just have one implementing function and all the others are simple wrappers around it.) A :revsort prefix could possibly be written inline as just :sort[some-filter] +[reverse[]], so you might be able to get away with not writing all the rev variations... but sorting and then reversing adds an extra O(N) step after the sort is finished, when that usually would have been quite easy to do inside the sort by just reversing the comparison function. So if you're going to write all those prefixes, adding a rev variant of each is probably worth it for the performance gains.

@saqimtiaz
Copy link
Member Author

@rmunn unless I can think of a better implementation, this will likely be a plugin for personal use and as such I would only write the prefixes that I find myself needing. With the way that I use TW that is pretty easy to plan in advance.

As you say the sheer number of prefixes required would make this a non-starter as a public plugin.

Part of the reason I wanted to bring this up now before 5.1.23 went live was in case I had missed a trick with the filter run prefix implementation, and maybe someone else might spot a more flexible and elegant solution.

@Jermolene
Copy link
Member

I like the idea of making things like "reduce" available in inline form. Having a profusion of filter prefixes doesn't feel bad because we can already be confident that the vast majority of users need never learn about their existence.

@saqimtiaz
Copy link
Member Author

@Jermolene I'll do some prototyping post 5.1.23 and we can then discuss the merits while looking at some code.

@rmunn
Copy link
Contributor

rmunn commented Dec 10, 2020

After thinking about this some more while writing #5246, I've come to the conclusion that filter run prefix suffixes, as ugly as that phrase is to type, might actually be necessary due to the fact that =1 =2 =3 :reduce[multiply<accumulator>] produces 0. There are two ways to make that produce the value you'd actually expect. One is to write it as =1 =2 =3 :reduce[<index>compare:number:gt[0]then<currentTiddler>multiply<accumulator>else<currentTiddler>]. The other is to allow a suffix (or second parameter) to filter run operators, so that you could write either =1 =2 =3 :reduce:1[multiply<accumulator>] or possibly as =1 =2 =3 :reduce[multiply<accumulator>],[1]. And I'm leaning more towards that being an option. Which would then allow for :sort:number, :sort:date, and so on.

@Jermolene
Copy link
Member

Notwithstanding my earlier comment, I agree that adding filter run prefix suffixes makes a lot of sense.

With reference to the clumsy nomenclature, perhaps this is our last chance to rename "filterrunprefix" to avoid the overloading of the term "prefix". We might use:

  • filterrunhead
  • filterrunmethod
  • filterruntype

Maybe the word "run" is redundant so we could have:

  • filterhead
  • filtermethod
  • filtertype

@saqimtiaz
Copy link
Member Author

saqimtiaz commented Dec 10, 2020

@Jermolene I would suggest filterruntype.

I don't think we should drop the word "run" as I could potentially see filtertype clashing with some hypothetical type attribute of filter operators in the future. Also our documentation already describes the sequences of filter steps as a filter run, so filter run type is more intuitive.

@Jermolene
Copy link
Member

I don't think we should drop the word "run" as I could potentially see filtertype clashing with some hypothetical type attribute of filter operators in the future.

That's rather my concern with "type": that it's too generic a term, and already littered through our code.

Also our documentation already describes the sequences of filter steps as a filter run, so filter run type is more intuitive.

Very good point.

Also to add:

  • filterrunjoiner
  • filterrunmodifier

@saqimtiaz
Copy link
Member Author

filterrunmodifier and filterrunmodifier suffix?

Do note that we do already use the term filter run prefix in docs for some time, but a change might be warranted here to avoid more confusion down the road.

@Jermolene
Copy link
Member

Do note that we do already use the term filter run prefix in docs for some time, but a change might be warranted here to avoid more confusion down the road.

Tsk I had forgotten, and thought we'd introduced the term with v5.1.23. I think the naming boat has sailed then, we'll have to live with it.

@saqimtiaz
Copy link
Member Author

saqimtiaz commented Apr 26, 2021

Now that we have support for suffixes for filter runs, I'll revisit the idea of a :sort filter run prefix as time allows.

I am considering the following syntax upon which I welcome input:

:sort:<type>:<reverse>[... filter ...]

Where <type> can have any of the following values:

  • string (also the default if type is not specified)
  • stringcs or cs (case sensitive string)
  • alphanumeric or an (alphanumeric)
  • number
  • integer
  • version
  • date

It is a bit tricky trying to have consistency with the sort and sortsub operators as they aren't consistent amongst themselves.

  • We have the following sort operators: sort[], nsort[], sortcs[], sortan[]
  • sortsub[] operator supports the following suffixes: string, number, integer, date, version

Thoughts? As a reminder, the use case for this is to allow filters like:

[tag[HelloThere]] :sort[get[text]length[]]

instead of:

<$vars compare-by-title-length="[length[]]">
[tag[HelloThere]sortsub:number<compare-by-title-length>]
</$vars>

@Jermolene
Copy link
Member

Thanks @saqimtiaz that seems sound to me.

@pmario
Copy link
Member

pmario commented Apr 26, 2021

It is a bit tricky trying to have consistency with the sort and sortsub operators as they aren't consistent amongst themselves.

Yea that's the burden of an "organically grown" filter system. ...

It's funny to see the differences.

TW V5.1.0

grafik

TW V5.1.24-prerelease

grafik

@saqimtiaz
Copy link
Member Author

saqimtiaz commented Apr 27, 2021

@Jermolene an issue regarding case sensitivity and consistency.

  • sort[] operator is case insensitive, which is why we have sortcs[]
  • sortsub:string[] is case sensitive. I wish I had caught this before sortsub[] was finalized so we could have had sortsub:string[] and sortsub:stringcs[].

So how should the prefix :sort:string[... filter ...] behave? case sensitive or insensitive?

My original intention was to have :sort:string[... filter ...] as case insensitive sorting, and
:sort:stringcs[... filter ...] as case sensitive sorting, but this would be the opposite of how sortsub:string[] works.


Alternative proposal is to use a suffix switch for case sensitivity which means it can be applied to :sort:number[] as well.

:sort:string,cs[... filter ...]

Without the cs flag the sorting would be case insensitive. Still not completely consistent with subsort[] but it is the best solution I can think of since we cannot change subsort[].

Edit: please do consider the matter of consistency with sort[] and subsort[] and not only preference for syntax.

@pmario
Copy link
Member

pmario commented Apr 27, 2021

I do like :sort:string,cs[... filter ...] best. There is much more visual difference

I think: :sort:string[...filter...] and :sort:stringcs[...filter...] IMO it's much more difficult for humans to see the difference.

@Jermolene
Copy link
Member

Thanks @saqimtiaz @pmario I do like the idea of pulling the case sensitivity flag into its own suffix; "stringcs" et al always feels like a bit of a hack.

@saqimtiaz
Copy link
Member Author

@Jermolene what should be the default for :sort:string[] when the case sensitive flag is not provided? It would be intuitive that the default is that the sorting is not case sensitive.

However note that subsort:string[] is case sensitive and the sort[] operator is not (though we have sortcs[])

@Jermolene
Copy link
Member

I think we may be stuck with having inconsistent defaults, so we should perhaps encourage the convention of always explicitly specifying the case sensitivity flag?

@rmunn
Copy link
Contributor

rmunn commented Apr 28, 2021

I think the default for the :sort prefix should match the default for the sort operator, not the default for the subsort operator. So case insensitive by default just like sort is. But all the examples in the documentation should explicitly state the sensitivity, and the docs should also say that if you leave off the sensitivity tag, the behavior is undefined and subject to change in the future.

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 a pull request may close this issue.

4 participants