Simplify parsing of facts with a clear tag barrier

[rhertzog]

This PR builds on top #662 to further simplify the parsing facts by creating a clear barrier between the description and the tags. But to be nice with the user, we support a subset of tags within the description that we copy over to the tag list and that we keep as plain words within the description.

[GeraldJansen]

Hmm, act@cat #tag1 gives category cat #tag1 and no tag. That would be a breaking change that some users might not appreciate.

Aside from that, I like the proposal and the code improvements.

[rhertzog]

Hmm, act@cat #tag1 gives category cat #tag1 and no tag. That would be a breaking change that some users might not appreciate.

Yeah. That was something that I deliberately changed in the test suite. Right now the code was allowing act@cat#tag even without any space. On the opposite, the code wanted to be very liberal on the names of activities and categories so I actually added tests to ensure that we could have spaces and hash in names of activities and categories. Task #1@Project #1 is now a valid activity/category which is a good move IMO.

I'd rather not add any special case to be backwards compatible on this specific input. It's not really possible to support this old syntax with the liberal names of categories. But if the consensus is that we can forbid hash in categories and activities, then I can do something but I'd don't believe that it would be a good move.

[rhertzog]

I added a supplementary change to ensure backwards compatibility with the double comma syntax, this is nice for external users (like GTG) who already adopted this change. And I got rrid of a few style issues that my code editor kept highlighting on me (unrelated to my changes, except that they are in the file that I modified).

[ederag]

Task #1@Project #1 is now a valid activity/category which is a good move IMO.

Just to lift any ambiguity: this was already possible with v3.0:

Nothing after the rightmost double comma: no tags.

[rhertzog]

Just to lift any ambiguity: this was already possible with v3.0:

Yeah, but at the same time here was a test to ensure that without the double comma it would parse entirely differently... it's exactly the kind of unexpected difference that create confusion for users.

[ederag]

without the double comma it would parse entirely differently...
it's exactly the kind of unexpected difference that create confusion for users.

Why would a user expect to get the same thing with or without the documented barrier ?

[rhertzog]

Why would a user expect to get the same thing with or without the documented barrier ?

Because a barrier ought to be not needed when you don't want to put anything after the barrier...

Anyway, I have been running this code for a while and it works nicely for me. @matthijskooijman and others, can you review and merge if you are satisfied ?

[ederag]

Because a barrier ought to be not needed when you don't want to put anything after the barrier...

If you "don't want to put anything after the barrier", there's no need for a barrier in the v3.0 rules:

[matthijskooijman]

@rhertzog, I've been meaning to look at this issue and others, but haven't found the time yet. Hopefully Soon(tm), but it is still on my mind.

[benjaoming]

I agree that this is a change for the better -- supposing that test cases are sufficient evidence that changes also work as intended.

[matthijskooijman]

I had an initial look at this, though not good enough to really grasp everything. Here's some initial thoughts, though:

• I like simplifying the parser, though with the extra tags-inside-description feature, the parsing gets a bit more complicated again.
• It looks like @GeraldJansen's first commit updates the documentation, but the subsequent commits by @rhertzog do not. I do think that they make changes that should be reflected in the documentation, right? This should be fixed before merging (but probably after we're sure how we want it exactly).
• I'm wondering if we should first focus on simplifying the parser and reverting the double comma requirement and delay the tags-inside-description feature for later? Though one commit message ("Obviously there are cases where we want to use tags within our description as they are part of it. We will restore this in the next commit.") seems to imply that they might be more linked, though I do not see how exactly yet.
• What are the restrictions introduced by this exactly? From the commit message I gathered that the activity can no longer contain a comma, that the description cannot contain a comma (or two) followed by a hash (maybe with spaces in between) and that tags must be space separated. Anything else?
• Am I understanding correctly that by using e.g. ,# as the separator between description and tags, you can still use commas inside the description (which is nice). Essentially this is what ,, was intended to solve, but using ,# neatly achieves the same, with only the less invasive limitation of disallowing ,#.

As I said, I haven't fully reviewed this yet (a bit too tired to grasp it completely right now), hope to get a closer look soon.

[rhertzog]

I like simplifying the parser, though with the extra tags-inside-description feature, the parsing gets a bit more complicated again.

I you don't do this, you lose some backward compatibility where you could put tags right after the description without any comma.

I do think that they make changes that should be reflected in the documentation, right?

Right, documentation updated.

I'm wondering if we should first focus on simplifying the parser and reverting the double comma requirement and delay the tags-inside-description feature for later?

I don't see why you want to delay any longer something that's ready. I would not be opposed in general if really required but given our delays of review of each small step, I don't think it's reasonable to self-inflict us this.

What are the restrictions introduced by this exactly?

The main restriction is that activity@category #tag no longer works the same way. You don't have any tag here, just a category named category #tag. Otherwise you go things right. If you look at the changes in the test suite, you can see where I had to make changes.

Am I understanding correctly that by using e.g. ,# as the separator between description and tags, you can still use commas inside the description (which is nice).

Yes. I just added a test to make this explicit:

    def test_description_with_commas(self):                                      
        activity = Fact.parse("case, meet with a, b and c, #holiday")            
        self.assertEqual(activity.description, "meet with a, b and c")

[matthijskooijman]

Thanks for your updates and response!

I don't see why you want to delay any longer something that's ready. I would not be opposed in general if really required but given our delays of review of each small step, I don't think it's reasonable to self-inflict us this.

My thought was that adding a new feature, such as parsing tags within the description, might need a bit more consideration than reverting the double comma, which we should do ASAP. But you're right, maybe I'm overly cautious and we should rather move forward.

Considering this PR a bit more, I think I approve of the changes it makes. There are some compatibility changes, but I indeed think it becomes more obvious for the user. I haven't looked close enough at the code yet to completely approve the PR.

@rhertzog, could you maybe have a stab at writing a changelog entry for this change? Given some things will no longer work with this version, I would think it to be useful to provide a verbose changelog entry for this, and I suspect you have the best overview of what changed exactly.

[GeraldJansen]

Given that this PR makes some breaking changes in the parsing of the Hamster activity strings, I'm pinging some people from clients that use Hamster:
@mwilck hamster-shell-extension (eg. readme Usage section: need a comma before tags too)
@aquaherd xfce-hamster-plugin (eg. projecthamster/xfce4-hamster-plugin#27)
@flavin, @diegogangl getting-things-gnome (not sure, but I believe tags are passed with Hamster plugin)

I've looked over the code/tests/doc changes and they look very good to me. I paid most attention to the tests and documentation because I'm not strong on regular expressions.

[mwilck]

@GeraldJansen, is there a summary of the new parsing rules somwhere?

[GeraldJansen]

@mwilck Simple version: activity name@category, description, #tag (note comma before tags).
Aside from that, see the input doc page and test cases.

[mwilck]

@GeraldJansen, thanks. This looks good to me. I never had much love for the double comma.

(the time/date parsing is still an issue I think, it lacks internationalization ("4PM") - but that's a totally independent topic).

[aquaherd]

No objections by me - I just pass through what the user enters onto the DBus method.

However I plan to check the result of the DBus call more thoroughly.

Can I assume that hamster will return some error text that I can display?

[ederag]

this basically converts a fact like "Coding, fix #bugs in #hamster"
into "Coding, fix bugs in hamster, #bugs #hamster"

This PR will eat all hashes in the description, and create tags instead.
I wouldn't want that.

This PR will also introduce numerous regressions, some mentioned in #657.

[WBTMagnum]

this basically converts a fact like "Coding, fix #bugs in #hamster"
into "Coding, fix bugs in hamster, #bugs #hamster"

This PR will eat all hashes in the description, and create tags instead.
I wouldn't want that.

I second that. I regularly use hashes in the description to reference tickets.

For me automatic conversion is troublesome, because I have to transfer the whole information into our companies time tracking at a later point. If the information is distributed across several fields, I have to re-combine that again.

One could argue, that it might be helpful to use the tags to filter specific tickets, but that does not work in my case either. I work with multiple ticketing systems, therefore a ticket number is not unique and I quickly have duplicates. No help.

For those reasons I don't use hamster's tag feature at all.

[GeraldJansen]

As far as I know there are two changes that could be considered regressions. 1) Commas can no longer be embedded in the activity name. This feature was requested by a single user and its low importance has already been discussed a few times. 2) People who use tags but not descriptions will now have to insert an extra comma before the tags. This has been mentioned above and does not seem to have created concerns for maintainers of clients.

[GeraldJansen]

Perhaps the automatic harvesting of tags from the description could be turned on or off by a boolean configuration setting (say "Extract tags from description"). It should likely be off by default given that it is a new feature. At any rate, it seems like the hash symbol should not be stripped.

[ederag]

People who use tags but not descriptions will now have to insert an extra comma before the tags.
This has been mentioned above and does not seem to have created concerns for maintainers of clients.

This is not only a regression from v3,
but also a strong backward incompatibility with v1 itself.
With this PR:

[ederag]

Commas can no longer be embedded in the activity name.
This feature was requested by a single user
and its low importance has already been discussed a few times.

The numbers are too low to evaluate the importance solely on the number of expressed requests.
Besides, #280 has been tagged as bug by someone else, so that makes at least three of us on the "important" side.
And majority can make mistakes, especially in an oligarchy of similar people.
Finally, there's much more to that.

[matthijskooijman]

I second that. I regularly use hashes in the description to reference tickets

The current code does not parse tags from the description that start with a number, for exactly this reason. Would that solve your concern? Or are you using some letter-based prefix that still causes problems here?

For me automatic conversion is troublesome, because I have to transfer the whole information into our companies time tracking at a later point. If the information is distributed across several fields, I have to re-combine that again.

So you are saying you are using #tags inside the description rather than the tags field and they'd need to stay there to simplify moving your facts to another system later?

If so, I guess that either or both of @GeraldJansen's suggestions might help here:

Perhaps the automatic harvesting of tags from the description could be turned on or off by a boolean configuration setting (say "Extract tags from description"). It should likely be off by default given that it is a new feature.

And:

At any rate, it seems like the hash symbol should not be stripped.

I was also thinking about the latter before, but didn't want to go into too much bikeshedding. Now that there is a workflow that would be helped by not stripping the tags from the description, I guess this might be worth considering.

[WBTMagnum]

I second that. I regularly use hashes in the description to reference tickets

The current code does not parse tags from the description that start with a number, for exactly this reason.

Ah, I didn't realize that.

Would that solve your concern?

Yes, that would resolve my personal concern, since I mainly handle number-based tickets.

Or are you using some letter-based prefix that still causes problems here?

I don't use letter-based tickets, but some people in my company do (e.g. from Jira). No hamster users though, but I try to convince them :-).

For me automatic conversion is troublesome, because I have to transfer the whole information into our companies time tracking at a later point. If the information is distributed across several fields, I have to re-combine that again.
So you are saying you are using #tags inside the description rather than the tags field and they'd need to stay there to simplify moving your facts to another system later?

No, I'm using descriptions like these #1234: fixed regressions stemming from #1112 or fixed some bugs [#111, #123, #155]. And in my workflow the description should stay unaltered and not be separated across different data fields.

But to be honest, the behavior is not new. I learned to work around this by either stripping the hashes myself oder wrapping the ticket numbers in brackets (e.g. [#111][#123][#155]). But of course, its a bit annoying ;-)

If so, I guess that either or both of @GeraldJansen's suggestions might help here:

Perhaps the automatic harvesting of tags from the description could be turned on or off by a boolean configuration setting (say "Extract tags from description"). It should likely be off by default given that it is a new feature.

That sounds like a feasible and reasonable solution to me.

And:

At any rate, it seems like the hash symbol should not be stripped.
I was also thinking about the latter before, but didn't want to go into too much bikeshedding. Now that there is a workflow that would be helped by not stripping the tags from the description, I guess this might be worth considering.

If automatic harvesting is optional, this is not a necessity to me. But it also sounds reasonable to leave the information intact.

[matthijskooijman]

It would be good to move this forward, but I'm not sure if we're there yet. I'm lacking time and brainspace to really dive into the code again for a full review, but I'll try to respond to previous comments at least.

@rhertzog wrote:

On the various suggestions from the last messages, I'm OK to keep the hash if that's what people prefer, however I'm strongly against the idea of a preference setting.

One additional (implied) suggestion would be to simply skip all automatic parsing of tags entirely, i.e. @mwilck wrote:

As @rhertzog has put it, the automatic parsing of tags from the description is just "being nice" to the user, i.e. a convenience functionality. And it turns out that quite some users speak up against it. I'd like to question whether something that's really just "nice to have" for some users, but an annoying mis-feature for others, is worth the effort at all.

And I wrote:

@rhertzog, is this autodiscovery something that you think is useful by itself and/or is it something that you would like to use? Or was this primarily intended as a way to add compatibility for adding tags after the description without commas (but then in a way that would make sense on its own still)?

I'm not entirely convinced that we really need this compatibility much though, we're changing the way how this parsing works, in a way that we think makes it better, so users will have to make some changes to their workflow in some cases, but I think that's ok.

@rhertzog, I think you haven't responded to these specifically yet? In particular, do you feel that auto-detecting tags is really added value, or just a mechanism to keep (partial) compatibility in the parsing rules? If the latter, would be inclined to omit it.

@rhertzog also wrote:

External clients sending a string should get the same result event stored no matter how the hamster application has been configured.

Is this really true? I would consider that the string-based interface is an interface between the user and hamster itself, and the external client is just a transparent pass-through? Then it makes sense that if the user configures hamster, then this interface between hamster and the user changes according to their preferences. If an external client wants to create a fact with fully predictable values, it should just submit a JSON fact instead? One caveat with this viewpoint is maybe when an external client wants to offer a string-based UI, but also apply autocompletion or suggestions in that UI, which does require that the external client know about the parsing rules (but maybe we have dbus calls for autocompletion already?).

Having said that, I do think that avoiding preferences where possible is a good strategy in general.

A bit longer ago, I also wrote this:

@rhertzog, could you maybe have a stab at writing a changelog entry for this change? Given some things will no longer work with this version, I would think it to be useful to provide a verbose changelog entry for this, and I suspect you have the best overview of what changed exactly.

I still think this is an important point, since we need the changelog entry before releasing a version with this change included, and I expect having a good writeup of user-visible changes will also help with the discussion around this PR before merging.

[rhertzog]

@rhertzog, I think you haven't responded to these specifically yet? In particular, do you feel that auto-detecting tags is really added value, or just a mechanism to keep (partial) compatibility in the parsing rules? If the latter, would be inclined to omit it.

I did reply to this here: #663 (comment)

It certainly adds value as I use it and I feel it's more natural to use it that way. At the same time, it's also a nice way to avoid most of the regressions.

I still think this is an important point, since we need the changelog entry before releasing a version with this change included, and I expect having a good writeup of user-visible changes will also help with the discussion around this PR before merging.

I propose:

• Improve the parsing of facts to detect the start of the tags part on the first comma followed by a hash (",#")
• Extract simple #hashwords from the description as additional tags (they can't start with numbers and can't contain spaces, those can only be written in the "tags part" that follows the first ",#")

[rhertzog]

Is this really true? I would consider that the string-based interface is an interface between the user and hamster itself, and the external client is just a transparent pass-through?

Yes it's true, here's an example:
https://github.com/getting-things-gnome/gtg/blob/master/GTG/plugins/hamster/hamster.py#L75

They build the string from the task and they submit it to Hamster with AddFact().

[matthijskooijman]

I did reply to this here: #663 (comment)

Ah indeed, missed that. Thanks.

It certainly adds value as I use it and I feel it's more natural to use it that way. At the same time, it's also a nice way to avoid most of the regressions.

Ok, so that would speak in favor of keeping the auto-tag-discovery as now implemented. I'm still not convinced that we need it for backward compatibility (reading more closely I realized that we're now keeping compatibility for tags directly following the description, but not for tags directly following the category, which was previously supported and is now intentionally dropped.

But if the feature is useful by itself for others (I have no need for it), I can live with having it. Any other opinions?

• Improve the parsing of facts to detect the start of the tags part on the first comma followed by a hash (",#")
• Extract simple #hashwords from the description as additional tags (they can't start with numbers and can't contain spaces, those can only be written in the "tags part" that follows the first ",#")

Ok, looks good, but I think it's incomplete. Rather than pointing out what I think is missing, I dove into the code again and tried to come up with a more complete suggestion:

• Improve handling of tags in the cmdline (i.e. the activity@category, description, #tag, #other tag string when you enter or edit an activity as a single string). In particular:
  • Remove the need for using a double comma to start the description a single comma can be used instead. Using double commas is still allowed for compatibility. This does mean that a comma can no longer be used in the activity or category.
  • The tags part must now be separated by a comma (previously, tags could be appended directly to the activity, category or description. This allows using # inside activity, category and description (without needing a double comma to force a possibly empty tags part at the end). For compatibility, the tags part can also be separated by a double comma.
  • Simple #hashwords are now extracted from the description as additional tags automatically (they can't start with numbers and can't contain spaces, but those can still be written in the "tags part" after a comma). This allows writing more natural descriptions with embedded tags, like Coding, fix #bugs in #hamster.
  • Limitations on the fields are more consistent. All fields can now contain any character, except:
    • The activity cannot contain @, since that would start the category.
    • The activity and category cannot contain ,, since that would start the description.
    • The description cannot start with # or contain the tag separator (one or two commas followed by optional whitespace followed by a hash, e.g. , #), since this would start the tags part.
    • Tags in the tag part cannot contain a # or , (since that would look like the start of the next tag).
    • Tags in the description part must start with a letter (upper or lowercase a-z) and cannot contain whitespace.

Does this look ok and complete?

If so, do we consider these changes acceptable? In summary, this removes the need for using a double comma and makes parsing a bit more predictable, at the expense of no longer allowing , in activity and category, ,# in the description, # at the start of the description and requiring the tags part to start with a ,. Of these I think only the first is a potentially significant limitation, but probably acceptable (looking back, I only used a comma inside an activity name once in the 9 years I have been using hamster).

[matthijskooijman]

Yes it's true, here's an example:
https://github.com/getting-things-gnome/gtg/blob/master/GTG/plugins/hamster/hamster.py#L75

They build the string from the task and they submit it to Hamster with AddFact().

Right, but I would argue that that client should be using AddFactJSON instead of relying on the user-targeted string (de)serialization scheme like they do now.

[rhertzog]

Does this look ok and complete?

Yes, your changelog entry looks good.

Right, but I would argue that that client should be using AddFactJSON instead

I agree but that's how it is. And it's still best to not deliberately break too much when we can avoid it.

[GeraldJansen]

If so, do we consider these changes acceptable?

Yes.

[WBTMagnum]

Upgrading to Ubuntu 22.04 brought the issues fixed in this PR back to my attention (e.g. double comma, tag removal from comments, etc.).

Since it's unclear if/when a new release will happen, I switched to rhertzog's code branch, following the steps below:

# Clone the official Hamster repository
$ git clone https://github.com/projecthamster/hamster.git
$ cd hamster

# Checkout rherzog's pull request #663
$ git fetch origin pull/663/head:fact-parser-changes
$ git checkout fact-parser-changes

# optional: clean-up (in case you already had a build)
$ ./waf clean distclean

# configure and build hamster package
$ ./waf configure build

# check for running hamster processes and stop them
$ pgrep -af hamster
$ pkill -f hamster-service
$ pkill -f hamster-windows-service

# confirm all hamster processes have been stopped
$ pgrep -af hamster

# install hamster (note: you have to use bash, fish will fail)
$ ( umask 0022 && sudo ./waf install; )

# optional: confirm functionality by adding a single comma separated command
$ hamster add "activity@work, test PR using a #tag within the comment and a single comma"

Maybe that's helpful to others. I'm running this setup for some time now and am very happy :-D.

[rhertzog]

Yeah, it would be really nice to see movement again and my branch merged. I realized I lost my own changes when I updated the Debian package to fix it for Python 3.11. :-(

[github-actions]

Automatically generated build artifacts for commit 2b5c059 (note: these links will expire after some time):

• Flatpak application: ⏬ Download

---

To test this PR, download and unzip the flatpak application above and then install with:

flatpak install Hamster.flatpak

[matthijskooijman]

I would also like to see this PR resolved before we release. Looking at previous discussion, I see everyone involved is positive about making these changes, except ederag. This change does make some backward incompatible change (people can no longer use comma's in their activity or category), but that seems acceptable IMHO (since it just adds a limitation to be observed by the user, compatibility with existing implementations that use the double-comma for serializing and submitting facts is kept).

I've rebased this PR (and force pushed to your branch @rhertzog, so pull carefully) to get a cleaner history when merging it, and so a new flatpak build is generated that allows easier testing. I want to do a few more tests - in particular what happens when an existing fact does have a comma in the activity, and to check if this change plays well with the gnome-shell extension.

[GeraldJansen]

I have pulled and tested this PR a little bit. An existing activity with embedded comma is displayed and reported just fine as long as it is not edited. As soon as you edit it the new parsing rules kick in. For example, "some,task@cat" becomes activity="some", category=None, description="task@cat". This is ugly but I think it is okay and will hardly affect any users given that Hamster didn't allow commas in the activity for most of its existence.

[GeraldJansen]

I also checked xfce4-hamster-plugin against this PR and did non find any weird behaviour. That was to be expected because the plugin simply passes the string to Hamster to be parsed.

[matthijskooijman]

Coming back to this PR after a while, it seems testing by @GeraldJansen was ok (and I think I did a bit of testing myself too earlier this year), so I think we can merge this as-is if nobody objects soon :-)

[rhertzog]

Coming back to this PR after a while, it seems testing by @GeraldJansen was ok (and I think I did a bit of testing myself too earlier this year), so I think we can merge this as-is if nobody objects soon :-)

Yay \o/