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
Propose alternate entry format optionally allowing descriptions ignor… #179
base: master
Are you sure you want to change the base?
Propose alternate entry format optionally allowing descriptions ignor… #179
Conversation
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.
I still need to be convinced that this is necessary.
And I definitely would require unit tests and documentation updates before I would consider merging it.
src/gtimelog/main.py
Outdated
self.notify('timelog') | ||
self.tick(True) | ||
|
||
def check_reload_tasks(self): | ||
if self.tasks.check_reload(): | ||
if self.tasks and self.tasks.check_reload(): |
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.
I've just seen this change in the other PR, haven't I?
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.
Yes. My mistake. I think I just forgot to go back to master before starting the new branch.
src/gtimelog/timelog.py
Outdated
# if category string can be further partitioned into non empty strings by '@' | ||
# use 1st as task and 2nd as category | ||
cat, task = c, t | ||
return cat, task |
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.
Given #181 you'll probably want to add a few .strip()s here and thee.
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.
Probably Yes. I wasn’t sure if you accept #181.
src/gtimelog/timelog.py
Outdated
@@ -283,6 +289,9 @@ def grouped_entries(self, skip_first=True): | |||
work = {} | |||
slack = {} | |||
for start, stop, duration, tags, entry in self.all_entries(): | |||
cat, task = self.split_category(entry) | |||
entry = task or '' | |||
entry = cat and cat + ': ' + entry or entry |
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.
I think task
is guaranteed to be a string, so the or ''
shouldn't do anything.
I would prefer the ternary if
expression instead of the x and a or b
hack.
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.
Task may also be None. See split_category()
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.
I would prefer the ternary if expression instead of the x and a or b hack
If you prefer, I can change this. Just want to point out that x and a or b
is not a hack but perfect valid and actually nice python syntax ;-)
Only fair. But currently this is just a proposal. To invest more time to make it nice and beauty I would like to have some tendency from you. I would regret using my weekend just to get this rejected based on general arguments against this. |
It’s natural and beautiful to write: vs
|
2b051b1
to
ee6f46f
Compare
Rebased onto master, changed condition syntax |
Uhh, the second looks more natural to me, although I'd put a # in front of the ticket number. And by having the project name in front you get to use completion. I need to sit down and figure out all the consequences of this. How would the reports change, how would grouping change. Would the task list need support for this syntax as well? I wonder if I could come up with a plugin API that would allow this kind of customization. |
ee6f46f
to
6f7e8fe
Compare
Currently the reports don’t change, because it’s still the task and the project returned from parsing. |
That may be a point but is also a matter of taste. If you work a lot with a tracker you might have the number at hand while working at a task, so starting to type the number is maybe faster for completion then starting with the project and sort out from all the previous tasks on that project.
I admit the 2nd looks a little bit nicer on reading. It still looks like this in the summary though. My main points are:
If we can do this with another logical separator, that would be fine too. |
JFYI, we’re using (with our gTimeLog fork a nested format of task names: Where |
Proposed solution for #91
I added some small changes to entry parsing for grouping to enable additional descriptions, that are ignored for grouping.
The change is backwards compatible with existing time log entries.
The additional format uses
@
to split task and category and changes the meaning of:
in the presence of a previous@
to split the general description from task and category.Format:
Example: