-
-
Notifications
You must be signed in to change notification settings - Fork 373
loosen tag restriction to allow all non-whitespace, after the first char #3961
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
Conversation
it looks like the test are failing on some python stuff:
File "/root/code/build/test/tag.test.py", line 612
self.t(f"add{'\x20+'.join(testtags)} one")
^
SyntaxError: f-string expression part cannot include a backslash
this works fine on my python. Looks like this was made to work in 3.13.6. I'll dumb it down presently.
...GothenburgBitFactory#3957) Instead of walking the string and stopping at the first non-isIdentifier() character (see Lexer::isIdentifierStart(), isIdentifierNext() and isSingleCharOperator()), walk all the way to the end of the word. This allows even punctuation and other characters to be used in tags. We still need to use isIdentifierStart() for the first character, to disambiguate it from a negative number or subtraction. Apparently there is no command context available, so the parser cannot "know" whether it's doing a "task calc" and have different parse rules. The lexing seems to happen before breaking the arguments down into commands for dispatch.
de3108a to
6a15239
Compare
for more information, see https://pre-commit.ci
ok, ready.
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.
This looks good! Please update task(1) to document this, where it describes the +tag|-tag syntax.
Please also make a corresponding update to https://github.com/GothenburgBitFactory/tw.org/blob/master/content/docs/tags.md and anywhere else you see fit in the docs.
Finally, Taskchampion will need updated to correspond -- see https://github.com/GothenburgBitFactory/taskchampion/blob/main/src/task/tag.rs#L8.
@djmitche so howcome the existing rust code doesn't reject the non-conformant tags coming from taskwarrior? There were no complaints from the storage backend with the C++ patch applied... that rust code you linked looks like it's supposed to bomb if the tag contains dashes.
I wondered the same thing! The short answer is, that particular code isn't used by Taskwarrior -- Taskwarrior just gets key/value maps from TaskChampion and does whatever it wants with them. The slightly longer answer is that TaskChampion treats any key/value map as valid and won't bomb, but would just ignore invalid tags. So even if someone had tags with characters in them that TC didn't like, and was interoperating with another tool using TC directly, nothing would explode -- they just wouldn't see those tags in the TC-based tool.
Uh oh!
There was an error while loading. Please reload this page.
This patch allows tags to be any non-whitespace characters, excepting the first char, which still must be an "identifier" character (see
Lexer::isIdentifier()), in particular so that "task calc" works with negative numbers and subtraction. Apparently CLI2 doesn't know the command yet when doing the parse, so it has the same rules for every command.I tried to get this to work on tags with spaces (for parity with Timewarrior), but could not get it working before I ran out of time to spend on this. That will have to be left as a future endeavor. I don't personally use spaces.
In the meantime, at least tags with dashes work now, and tags can also use utf8 chars, including as the first/only character, if they wish.
The patch includes a few extra tests using tags with dashes and glyphs.
Note: I did not test sync.
Fixes #3957