Skip to content

WIP: Add complex tag filtering#462

Closed
limdingwen wants to merge 2 commits into
GothenburgBitFactory:refactor/filteringfrom
limdingwen:tag-operators
Closed

WIP: Add complex tag filtering#462
limdingwen wants to merge 2 commits into
GothenburgBitFactory:refactor/filteringfrom
limdingwen:tag-operators

Conversation

@limdingwen
Copy link
Copy Markdown
Contributor

@limdingwen limdingwen commented Nov 10, 2021

Continuation of #456. This WIP PR aims to add complex tag filtering, allowing for boolean operators to be done on tags. Example:

Screenshot 2021-11-11 at 12 13 56 AM

TODO:

  • Refactor code such that boolean parser is possible
  • Add boolean parser, based off of taskwarrior complex filtering?
  • Add basic OR/- support as a test
  • Add feature flag
  • Add documentation
  • Add tests

Would close #209.
Would close #64.
Would close #358.

Signed-off-by: Lim Ding Wen <limdingwen@gmail.com>
Signed-off-by: Lim Ding Wen <limdingwen@gmail.com>
@limdingwen
Copy link
Copy Markdown
Contributor Author

src % timew config complexFiltering off     
No changes made.
src % timew summary school OR work :yesterday
No filtered data found in the range 2021-11-10T00:00:00 - 2021-11-11T00:00:00 tagged with OR, school, work.
src % timew config complexFiltering on       
Are you sure you want to change the value of 'complexFiltering' from 'off' to 'on'? (yes/no) yes         
Config file /Users/ignis/.timewarrior/timewarrior.cfg modified.
src % timew summary school OR work :yesterday

Wk  Date       Day Tags      Start      End    Time    Total
W45 2021-11-10 Wed work    0:13:55  0:56:52 0:42:57
                   work   10:37:59 12:04:13 1:26:14
                   school 12:34:33 13:39:19 1:04:46
                   [snip]

Feature flag added under complexFiltering.

Copy link
Copy Markdown
Member

@lauft lauft left a comment

Choose a reason for hiding this comment

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

Nice POC! 👍🏻 See also the comments in the code.

Comment thread src/commands/CmdChart.cpp
auto filtering = IntervalFilterAndGroup ({
new IntervalFilterAllInRange ({ filter.start, filter.end }),
new IntervalFilterAllWithTags (filter.tags())
new IntervalFilterAllWithTags (filter.tags(), rules.getBoolean("complexFiltering"))
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I would not make this a top-level configuration (like e.g. verbose), but put it into a commands subgroup. I propose commands.filtering with values simple (= default) and complex (or maybe better: boolean).

for (auto& tag : _tags)
{
if (! interval.hasTag (tag))
if (_complexFiltering) {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

As you have already said, this should not be part of IntervalFilterAllWithTags, but rather some function or factory which translates the individual command line arguments into filters or filter groups, like e.g. IntervalFilterOrGroup, IntervalFilterWithoutTagSet, etc.

for (auto& tag : _tags)
{
return false;
if (tag == "OR")
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Currently, I would opt for lowercase, i.e. or

@lauft
Copy link
Copy Markdown
Member

lauft commented Nov 14, 2021

Thanks for the update, @limdingwen !

Based on my understanding, this current version would not support boolean parsing yet due to CLI::getFilter(Range) returning an Interval which does not contain ordered information, and more work needs to be done there. Is it intended such that CLI::getFilter(Range) will return a IntervalFilter instead to aid in this?

This is not just a simple replacement of CLI::getFilter(...), there are also the error cases (e.g. missing id, wrong number of ids) and their output which have to be handled correctly. Moving the parsing into a function of CLI is a goal, but only the final step. So at first, I would implement the construction of the filtering within each command, even if this means code duplication at first.

Therefore as a first step, I would split CLI::getFilter(<default range>) : Interval at first into CLI::getRange(<default range>) : Range and CLI::getTagSet() : std::set<std::string>. From there we can then adapt each command for the new filtering.

Depending on the respective command (see table in this comment) we can then compose the appropriate filtering. For the new filtering another CLI-function is required, returning a vector of strings from which we then can create the complex tag filter.

Later all this could be combined into some kind of IntervalFilterFactory (and maybe moved to CLI), but I would wait with this until we have a full overview how the implementation in each command looks like.

@sruffell
Copy link
Copy Markdown
Contributor

@lauft, @limdingwen : I might have missed it, but I was wondering if the refactor/filtering branch is going to be submitted via a separate PR?

@lauft
Copy link
Copy Markdown
Member

lauft commented Nov 17, 2021

@sruffell I am currently doing some performance tests to check whether something deterioated with refactor/filtering. If I spot no errors, I will merge it to dev.

@limdingwen
Copy link
Copy Markdown
Contributor Author

Hey, just an update -- currently doing finals and some other personal projects, would come back to this when I have the time 😄

@lauft lauft mentioned this pull request Dec 2, 2021
@lauft
Copy link
Copy Markdown
Member

lauft commented Dec 2, 2021

@limdingwen FYI: I have merged the refactoring of interval filtering to dev. I'd be happy to see you to pursue this WIP further.

Good luck for your finals! 🤞🏻

@lauft lauft deleted the branch GothenburgBitFactory:refactor/filtering December 2, 2021 21:46
@lauft lauft closed this Dec 2, 2021
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.

3 participants