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

Parser updates #482

Merged
merged 39 commits into from
Dec 27, 2019
Merged

Parser updates #482

merged 39 commits into from
Dec 27, 2019

Conversation

ederag
Copy link
Collaborator

@ederag ederag commented Nov 20, 2019

The first commit is #476.
The next two were from #475 (that will be closed).

Breaking change:
double comma ,, is now always required to start a description.
Single comma does not work any longer for this purpose.
If a single comma happens to be used by mistake,
the comma and the following text would end in the category.
So comma will be forbidden in the category,
to raise an error instead of merging a wrong fact in the database.
That will be the purpose of a forthcoming PR.

This allows to enter a comma in the activity (fixes #280),
while even clarifying the parser.

Then with a double comma barrier ,, before tags,
hashes can be anywhere in the activity, category or description.
(fixes #423)

The same parser is used in gui (cmdline) and terminal (command line).
(Fixes #438)
The only difference is the location of start-end: head or tail, respectively (no change).

New: dates can be entered on the gui cmdline.
Otherwise facts overlapping day-start would be impossible to enter due to #476.
For standard facts that fit in a single hamster day, times are enough (no change).

Almost finished, just need

  • documentation update
    (meanwhile, a good way to investigate: enter in the separate fields and look at the cmdline)
  • cleanup code (could be easier to navigate)

With a double comma before tags,
hashes can be anywhere in the activity, category or description
@GeraldJansen
Copy link
Contributor

GeraldJansen commented Nov 20, 2019

I played with this a little and it seems okay to me.

@ederag ederag changed the title Parser updates WIP: Parser updates Nov 23, 2019
This is breaking: starting a description with a single comma
does not work any longer. A double coma is required instead.
@ederag
Copy link
Collaborator Author

ederag commented Nov 23, 2019

9cce2d7 allowed both a single and a double comma to start a description.
That fixes #280 and #423 in the gui.
Unfortunately, that has several drawbacks, for instance
a fact with a comma in the activity (#280) and no description would need two double commas,
e.g. activity 1,231,, ,, #tags
And 231 goes to the description until ,, is finally entered.

Restricting the description barrier to be a double comma ,, and not , any longer
yields a much cleaner code and behavior IMO (42d3c05)

Since the next version is a major revision anyway, this might be acceptable.
There is no change for facts without description that comply with the old syntax (no # or ,).
If there is a strong pressure to keep the existing behavior,
then the description barrier will have to be configurable,
but that will increase code and testing complexity a lot.

Kind word of warning:
please do not waste our energies trying to advocate at length for just keeping the old parser,
this is not going to happen.
It is already clear that some people are not interested in this change.
But others are, and I do want to remove these limitations.

With this change # and , are finally allowed in activity, which will fix #280 and #423 in the gui.

known issue (loosely related): in the gui cmdline, do not try to add a comma in a tag;
that is not allowed, and is currently wreaking havoc in the cmdline.

@GeraldJansen
Copy link
Contributor

GeraldJansen commented Nov 25, 2019

I've played with this a bit and it seems to work "as advertised".

A hiccup I've noticed: when entering the old style task@category, some description, the description gets silently swallowed. That's a bit nasty for users that upgrade and aren't aware of the change.

@ederag
Copy link
Collaborator Author

ederag commented Nov 26, 2019

Good catch, thanks ! 50ab84d makes the error slightly more obvious since
the category becomes category some description.
Next step is to warn about the comma instead of swallowing it.

Do not silently swallow commas in category.
Get datetimes directly, using regular expressions,
following parse_datetime_range in hamster-cli.

The parser is both simplified further and enhanced.
Civil dates can now be given explicitly, as on the command line.
@ederag
Copy link
Collaborator Author

ederag commented Dec 1, 2019

This is still experimental,
but the parser is almost finalized.
Using almost the same method as in hamster-cli, that fixes #438 as well.

def test_roundtrips(self):

gives an idea of the possibilities.
Next step: polish, and use the same parser for command line
(slightly modified to look for the datetimes at the "tail" instead of "head").

Necessary for past days in gui,
while keeping relative times reference to be now.
@ederag
Copy link
Collaborator Author

ederag commented Dec 8, 2019

Done: the cli now uses the main parser
(same as the gui, albeit with the range looked for at the end, as justified in the previous comment).
Still WIP though, as facts overlapping day start can not be entered since #476.
That will be fixed by displaying date(s) in the serialized string, if required.

@ederag
Copy link
Collaborator Author

ederag commented Dec 9, 2019

That will be fixed by displaying date(s) in the serialized string, if required.

Done in d8c6425.

Breaking:
- removed prepend_date argument,
- renamed serialized_time to serialized_range.
@ederag ederag changed the title WIP: Parser updates Parser updates Dec 15, 2019
@ederag ederag mentioned this pull request Dec 22, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants