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

Support multiline entry summaries #27

Merged
merged 6 commits into from
Mar 31, 2022
Merged

Conversation

vladdeSV
Copy link
Owner

Add syntax support for multiline comments.

will break in the specific scenario: user it indenting with 2 spaces,
and have their multiline comment start with 4 spaces, and the first
character in their multiline comment starts with a number
@vladdeSV
Copy link
Owner Author

@jotaen When available, is there a chance you could provide me an early preview Windows build before the release of klog with multiline support?

I would appreciate if I could toy around with the multiline syntax highlighting using a binary which supports it.

@vladdeSV
Copy link
Owner Author

vladdeSV commented Feb 27, 2022

As mentioned in a commit, the syntax highlighting breaks in one specific scenario: if the user begins a multiline comment with four spaces and the first character is a number.

This is because I currently cannot* determine the indention style used with the record. If a user indents with two spaces, then correct highlighting should apply when the user starts writing after four spaces. However, if indenting with four spaces, then I do not want the highlighting to appear when writing a time; it looks ugly.

Since an entry must start with a number, the rule is as follows: "if a row starts with more than 4 spaces, and the next character is not a number, it is a multiline comment row".

image

* I think I could use some regex magic ranges which looks at the indentation of the first row and only works for that type of indentation. This will take time, but I think I could make it work.

@jotaen
Copy link

jotaen commented Feb 27, 2022

The code for multiline summaries is already finished and in the main branch. Are you able to compile klog from sources? With go 1.17 setup, it’s relatively straightforward to compile the binary for your platform. That would probably be easiest. Otherwise let me know, then I can create a pre-release with all binaries.

The syntax highlighting issue is unfortunate indeed, and as you said, “reverse-engineering” the indentation style is probably not a good idea (entries could also start with - or +). However, the klog json command could include the indentation style information in its output, because the klog parser already has that at hand. So how about an additional meta-section, which is included in each record’s JSON, like so:

"formatting": {
  "indentation": "    "
}

It wouldn’t be much work on my end to make that happen. If needed, we could even extend it, basically all of the following information is currently available:

  • Line ending style (\n vs \r\n)
  • Indentation (the exact sequence, e.g.      or \t)
  • The preferred date and time format (YYYY/MM/DD vs. YYYY-MM-DD or 12-hour vs 24-hour clock)
  • Whether the user uses spaces around the dash in ranges (12:00-13:00 vs 12:00 - 13:00)

The last two are mainly useful when klog manipulates the file. E.g. when doing klog start, it figures out what style you use in the file, and respects that for the newly created entry.

@jotaen
Copy link

jotaen commented Feb 27, 2022

Another topic that came to my mind is the structure of the output of summaries in the JSON. Currently, it’s one string, where the individual lines are joined via \n. It would also (additionally) be possible to provide an array of lines instead, e.g.

"summary": "Foo\nBar",
"summaryLines": ["Foo", "Bar"]

For the record:

2020-01-01
    1h Foo
        Bar

That’s both applicable for record summaries, and for (multiline) entry summaries. Would something like that be useful or is the current representation sufficient?

@vladdeSV
Copy link
Owner Author

The code for multiline summaries is already finished and in the main branch.

Awesome. I'll compile the main branch 👌.

So how about an additional meta-section,

Unfortunately I do not know how to match metadata with the syntax highlighter. I am pretty sure this is possible, but I lack the knowledge of how to do so.

Currently I do not think you should put effort into making this. However, a very cool idea, which I'd like to expand on in the future!

entries could also start with - or +

Ah, that is true. Until I figure out matching metadata with the syntax highlight I'll have to resort to brute force methods in order to provide a nice user experience while editing the files manually.

I personally only use VS Code to edit the files manually (it's a nice and readable format!), then use the binary to calculate times.

Would something like that be useful or is the current representation sufficient?

As of right now, I never look at the output of the extension. Only errors are used. Some planned features will, like #25. However, I don't think it would be necessary. (.split('\n') is sufficient)

@vladdeSV
Copy link
Owner Author

[…] all of the following information is currently available:

  • Indentation (the exact sequence, e.g. or \t)
  • The preferred date and time format (YYYY/MM/DD vs. YYYY-MM-DD or 12-hour vs 24-hour clock)
  • Whether the user uses spaces around the dash in ranges (12:00-13:00 vs 12:00 - 13:00)

As mentioned, I will not use this right now. However, it's good to know that it could be provided.

Although I have not made an issue for it yet, sometime in the future I want to implement a "format document" command, which for instance would allow indentation to be auto (get from klog), tab, 4 space, 3 space, or 2 space. Same for the others you mentioned as well.

@jotaen
Copy link

jotaen commented Feb 28, 2022

Unfortunately I do not know how to match metadata with the syntax highlighter.

Ah right, I forgot that the syntax highlighting is based on regexes, not on the JSON output. But at least it should be rare for a subsequent summary line to start with like 60m, so it hopefully isn’t a big issue in practice.

@vladdeSV vladdeSV merged commit a5aeb1d into main Mar 31, 2022
@vladdeSV vladdeSV deleted the patch/multiline-entry-summary branch March 31, 2022 16:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants