Implementation changes for tag filtering

[sytone]

Based off feedback from PR and tracked in #632

• I think it should be possible to search for tags without adding the leading # to the query.
• I think the search should not be case-sensitive.
• I think the search should match as a substring of the tag (to allow includes house/ for #house/chores and #house/interior).
• Similarly, not should be the negation of the above (not even match a sub-string, case-insensitive).
• I think the global tag should be completely disregarded, as if it didn't exist (for filtering, sorting, …).
• I think the filter should be tags include house/ rather than tag includes …
  Originally posted by @schemar in Add tags as property of task #631 (comment)

Also

• Write documentation - the Add tags as property of task #631 (comment).

Added:
Ability to sort by more than first tag. Sort by takes an index number and if the collection of tags has a valid length will use the tag at the index for the comparison. By default will used first (index 0) Also handles comparison of tasks that do not have tags.

[sytone]

Will add the separate items to this PR as I work though them.

[sytone]

Can you assign the issue to me so I can mark the items off as I complete them.

[claremacrae]

Can you assign the issue to me so I can mark the items off as I complete them.

Done - and thank you!

[claremacrae]

Hi @sytone
I know you aren't ready for code review yet, and I spent far too much time agonising over whether to send this now or not ...

But since you are going to be writing more tests, I thought it kinder to send feedback on the tests sooner rather than later... to your time and mine!

Cheers.

[sytone]

I normally use UnitOfWork_StateUnderTest_ExpectedBehavior as the name for tests as my primary language is C#. I would also have a disposable class for setup that is used in each test.

JavaScript is always an adventure in our know the language can do x as I did it before but for the life of me I cannot remember x implementation. So happy to take any language feedback.

[schemar]

Thank you for picking this up @sytone!
Thank you @claremacrae for your review!

[claremacrae]

I would also have a disposable class for setup that is used in each test.

Yes, totally agreed!

I've just asked in the #plugin-testing thread on Discord, to see if anyone can advise on how to do that sort of thing in Jest:

https://discord.com/channels/686053708261228577/962362830642905148/971457623222263811

[claremacrae]

Thank you very much for this. I’m trying to get the ‘group by’ PR done, then will come to this and try it out!

[sytone]

Ok, all changes are in for this update now.

[claremacrae]

Many thanks for this set of changes, and this very useful functionality. I hope my comments make some kind of sense!

[claremacrae]

Many thanks for this set of changes, and this very useful functionality. I hope my comments make some kind of sense!

@schemar I’m really conscious that this is quite a thorough review and it’s very likely that users won’t notice most of the things I pointed out for quite some time.

I wonder you might comment on my comments, and suggest any that you feel are not worth worrying about to delay the first release of an undoubtedly useful feature - and perhaps might be moved to separate issues for optionally dealing with at a later stage?

[schemar]

Thank you for the review @claremacrae! I didn't review the entire PR. I only answered to your comments and said where I think changes make sense or not.

@sytone please also let us know what your motivation levels are. Maybe we can do the last changes if you don't think you can make it.

[claremacrae]

Re enabling the linting, I'm generally a bit cautious about turning on a new check on a PR that mixes other changes...

Am struggling to express why, other than a general feeling that it may make it harder to merge other existing PRs, or PRs taken from before this one was branched, due to the potential for failing checks on markdown formatting, and if someone adds markdown formatting to an older branch, we'll have all manner of conflicts to deal with)

Anyway, in a roundabout way, this prompted me to try manually merging this branch with --squash on my machine, to see what the overall changes would look like...

Can someone tell me what I'm doing wrong? (Or what I have misunderstood!! 😆 )

Here's what I did:

# Prepare to merge
git checkout main
git pull
# Use webstorm to get this PR on to my machine, as branch sytone_main

# Try doing the merge
git merge --squash sytone_main
git commit -m 'Stuff'

# Discover that I needed to add new dependencies
yarn

# Try doing the merge
git commit -m 'Stuff'

# pre-commit hooks failed:

markdownlint-cli2-fix v0.4.0 (markdownlint v0.25.1)
Finding: **/*.md !node_modules src/Query.ts src/Sort.ts src/Task.ts tests/Query.test.ts tests/Sort.test.ts tests/Task.test.ts
Linting: 30 file(s)
Summary: 8 error(s)
src/Query.ts:1 MD041/first-line-heading/first-line-h1 First line in a file should be a top-level heading [Context: "import * as chrono from 'chron..."]
src/Query.ts:28:1 MD037/no-space-in-emphasis Spaces inside emphasis markers [Context: "    private _"]
src/Sort.ts:1 MD041/first-line-heading/first-line-h1 First line in a file should be a top-level heading [Context: "import type moment from 'momen..."]
src/Task.ts:1 MD041/first-line-heading/first-line-h1 First line in a file should be a top-level heading [Context: "import type { Moment } from 'm..."]
src/Task.ts:80:42 MD011/no-reversed-links Reversed link syntax [([\s\t]*)[-*]]
tests/Query.test.ts:1 MD041/first-line-heading/first-line-h1 First line in a file should be a top-level heading [Context: "/**"]
tests/Sort.test.ts:1 MD041/first-line-heading/first-line-h1 First line in a file should be a top-level heading [Context: "/**"]
tests/Task.test.ts:1 MD041/first-line-heading/first-line-h1 First line in a file should be a top-level heading [Context: "/**"]
error Command failed with exit code 1.
info Visit https://yarnpkg.com/en/docs/cli/run for documentation about this command.

[claremacrae]

Can someone tell me what I'm doing wrong?

What I don't understand is that the GitHub Actions all seem to have passed.
Are the checks running on the merged code?
Have I done something wrong in my attempt at command-line merging?

[sytone]

Did you checkout from a clone of my repo? I can pull the markdown linting changes out and push them to another PR. Just added them as it was part of comments for this PR. Will creation new issue and move them.

[claremacrae]

Did you checkout from a clone of my repo?

Yes

I can pull the markdown linting changes out and push them to another PR. Just added them as it was part of comments for this PR. Will creation new issue and move them.

Before you do so, how will that interact with all the other changes in this PR please?

Can it still not turn on enforcing of linting automatically, until we have had a chance to see a few successful merges of branches to main? It's better to turn on enforcement of checks incrementally, instead of all-in-one.

[claremacrae]

I think it might simplify things if in future you created branches in your repo, instead of making all your changes on your main????

[claremacrae]

@sytone Before you go ahead and make more changes, I wonder, would you be interested in pairing for a while to explore what's going on on my machine?

We got close to this PR being mergeable, and it would be a shame for you to have to re-do work, and us to have to re-do reviews, for what could well be a simple mistake on my part!

[sytone]

I just reverted all the lint changes, that will com in on a separate PR and branch, I usually use branches but just expected one change :) moving over to that model and following issue format. VSCode makes it simpler with the github integration.

[sytone]

Linting changes for Markdown are in PR #660

[claremacrae]

Can someone tell me what I'm doing wrong? (Or what I have misunderstood!! 😆 )

Just to confirm that now that the markdown linting changes have been reverted, the following steps all work fine, on revision 67e97c6 🎉

Here's what I did:

# Prepare to merge
git checkout main
git pull
# Use webstorm to get this PR on to my machine, as branch sytone_main

# Try doing the merge
git merge --squash sytone_main
git commit -m 'Stuff'

# Discover that I needed to add new dependencies
yarn

# Try doing the merge
git commit -m 'Stuff'

# pre-commit hooks failed:

markdownlint-cli2-fix v0.4.0 (markdownlint v0.25.1)
Finding: **/*.md !node_modules src/Query.ts src/Sort.ts src/Task.ts tests/Query.test.ts tests/Sort.test.ts tests/Task.test.ts
Linting: 30 file(s)
Summary: 8 error(s)
src/Query.ts:1 MD041/first-line-heading/first-line-h1 First line in a file should be a top-level heading [Context: "import * as chrono from 'chron..."]
src/Query.ts:28:1 MD037/no-space-in-emphasis Spaces inside emphasis markers [Context: "    private _"]
src/Sort.ts:1 MD041/first-line-heading/first-line-h1 First line in a file should be a top-level heading [Context: "import type moment from 'momen..."]
src/Task.ts:1 MD041/first-line-heading/first-line-h1 First line in a file should be a top-level heading [Context: "import type { Moment } from 'm..."]
src/Task.ts:80:42 MD011/no-reversed-links Reversed link syntax [([\s\t]*)[-*]]
tests/Query.test.ts:1 MD041/first-line-heading/first-line-h1 First line in a file should be a top-level heading [Context: "/**"]
tests/Sort.test.ts:1 MD041/first-line-heading/first-line-h1 First line in a file should be a top-level heading [Context: "/**"]
tests/Task.test.ts:1 MD041/first-line-heading/first-line-h1 First line in a file should be a top-level heading [Context: "/**"]
error Command failed with exit code 1.
info Visit https://yarnpkg.com/en/docs/cli/run for documentation about this command.

[claremacrae]

Can someone tell me what I'm doing wrong? (Or what I have misunderstood!! 😆 )

Just to confirm that now that the markdown linting changes have been reverted, the following steps all work fine, on revision 67e97c6 🎉

(I would still like to understand why the previous changes passed the GitHub Actions CI on the PR, but failed in a developer build - in other words, why the GitHub Actions failed to spot the (probable) issue in a PR....
But that's not relevant to this PR! 😀 )

[sytone]

Yeah, not sure on that one. Lets see if the other PR has the same issues.

[sytone]

Thank you for the review @claremacrae! I didn't review the entire PR. I only answered to your comments and said where I think changes make sense or not.

@sytone please also let us know what your motivation levels are. Maybe we can do the last changes if you don't think you can make it.

I was away for the week with work. This should all be covered now. Stepping away for a while, will look any other comments in the next few days. I have most things covered.

[claremacrae]

I just noticed a couple of things in the docs...

[sytone]

Doc changes were completed.

[claremacrae]

Brilliant work. I've reviewed all the recent changes and there definitely look good to me...

Approving, and will go ahead and merge.

As I'm sure @schemar will also say, very many thanks thanks for all your work one this useful feature. 😄 🎉

[claremacrae]

This was included in the 1.6.0 release.