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

Forbid unparseable facts #476

Closed
wants to merge 3 commits into from

Conversation

ederag
Copy link
Collaborator

@ederag ederag commented Nov 16, 2019

Make the save button active only if the fact entered can be parsed (and thus edited) from the cmdline.
This hopefully answers #461 (comment) favorably, while waiting for #465.

The first two commit are from PR #475, only a544d5e is added by this PR.

@GeraldJansen
Copy link
Contributor

GeraldJansen commented Nov 17, 2019

Thanks! Yes this solves #461 (comment) reasonably well, as well as the two issues closed by the previous two commits.

I noticed that the parser changes allow "#" in any position in the activity name, in addition to the first, which is fine with me. Playing with this a bit, I noticed some quirky behaviour. For example, typing #123 and #456@bugs, description slowly in the cmdline, 456@bugs is considered a tag up until the ', d' is entered and then flips to be part of the description and category. I guess this is a corner case that can be addressed later under #465.

(ps. very glad you're back!)

@ederag ederag added the design Design discussions and interesting use cases label Nov 17, 2019
@ederag ederag added this to the v3.0 milestone Nov 17, 2019
@ederag
Copy link
Collaborator Author

ederag commented Nov 17, 2019

@GeraldJansen

56@bugs is considered a tag up until the ', d' is entered and then flips to be part of the description and category

Thanks for the feedback, glad you're still there !
That kind of interpretation change seems inevitable since
activity #456 was interpreted as an activity with 456 tag,
and we want to be fully backward compatible.

Just noticed: adding a comma in the middle of a tag in the gui cmdline wreaks havoc. 😞

@@ -23,7 +23,7 @@
tag_re = re.compile(r"""
[\s,]* # any spaces or commas (or nothing)
\# # hash character
([^#\s]+) # the tag (anything but # or spaces)
([^#\s,]+) # the tag (anything but #, spaces or comma)
Copy link
Contributor

@GeraldJansen GeraldJansen Nov 18, 2019

Choose a reason for hiding this comment

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

This change would fix the strange behaviour with #456@cat:

    ([^#@\s,]+)  # the tag (anything but #, @, spaces or comma)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Wouldn't that forbid '@' in tags and be a regression ?
IMO the behavior is certainly puzzling at first sight, but clearly understandable
if the rules can be kept simple.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, so we also need to allow @ in the middle of tags. I never would have imagined that. Okay.

@GeraldJansen
Copy link
Contributor

Looking at the Input help page (eg. here), it seems that "#tag with spaces" is explicitly mentioned. This no longer works with this PR (or maybe earlier). For cmdline input abc@cat, desc #tag with spaces the description becomes "desc #tag with spaces" and there are no tags. A regression?

@ederag
Copy link
Collaborator Author

ederag commented Nov 19, 2019

Perfect, that used to work in v2.0 (c3e5fb7), indeed;
it would have been a pity not to allow spaces in tags as well 😄

edit:
Well, this is not compatible with 9fe4518

activity = Fact("case, with added #de description #and, #some #tags")
self.assertEquals(activity.activity, "case")
self.assertEquals(activity.description, "with added #de description")
self.assertEquals(set(activity.tags), set(["and", "some", "tags"]))

So parser backward compatibility seems impossible to achieve.

This was referenced Nov 19, 2019
@ederag
Copy link
Collaborator Author

ederag commented Nov 23, 2019

superseded by #481

@ederag ederag closed this Nov 23, 2019
@ederag ederag deleted the forbid-unparseable branch January 25, 2020 18:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
design Design discussions and interesting use cases
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants