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

Added _get_task_attrs method to backend #50

Conversation

lyz-code
Copy link
Contributor

Hi, I'm adding the feature of being able to parse the history of taskwarrior
(through the undo.data).

One application of this feature that I'm also writing is the ability to measure
the total time spent on tasks, without the ugly hack of adding an annotation
each time you start or stop a task. That info is already in the undo.data.

So, instead of a huge MR I'm splitting it to be somewhat edible :)

I need this method to parse the available task attributes of the undo.data file.

I've also added a minimum support of UDAs detection of the config, it doesn't
solve issue #30 but it's
a start.

Cheers

@coveralls
Copy link

coveralls commented Jan 26, 2018

Coverage Status

Coverage increased (+0.1%) to 91.72% when pulling 10e2f03 on lyz-code:feature/get_task_attributes into 79e9a98 on robgolding:develop.

* Added tests for active_time method
@jamatute
Copy link

Hi @robgolding, is there anything I can do to help with the review of the Pull request?

@robgolding
Copy link
Collaborator

Sorry for the delay, I'll go over this now.

Copy link
Collaborator

@robgolding robgolding left a comment

Choose a reason for hiding this comment

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

Not really sure what the purpose of this PR is @lyz-code—perhaps if it had more context and the usage of the new method was shown it would be easier to review and suggest an implementation.

Thanks so much for the contribution to the project, look forward to seeing what you suggest!

@@ -245,7 +245,7 @@ def filter_class(self):

@property
def config(self):
# First, check if memoized information is available
# First, check if memorized information is available
Copy link
Collaborator

Choose a reason for hiding this comment

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

This isn't a typo 😄 it refers to memoization (I know, weird word!)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ouups, sorry, I didn't know the word nor my corrector.

Restored on cf4d52a

@@ -427,3 +427,34 @@ def valid(output):

def sync(self):
self.execute_command(['sync'])

def _get_task_attrs(self):
TASK_STANDARD_ATTRS = [
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think this would be good to define on the class itself (either the backend or Task) rather than in the method.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Moved on commit 7805380

if 'uda' in index:
udas.add(re.sub(r'.*uda\.(.*?)\..*', r'\1', index))
available_task_attrs.extend(udas)
self.available_task_attrs = tuple(available_task_attrs)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Since the method is a "getter", it should return something rather than just set an attribute on the object.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You are right, I changed it to _set_task_attrs on 7805380

available_task_attrs = TASK_STANDARD_ATTRS
udas = set()
for index in self.config:
if 'uda' in index:
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should this not check for index.startswith('uda.')?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yup, fixed in f055fce @robgolding

@lyz-code
Copy link
Contributor Author

lyz-code commented Mar 6, 2018

I didn't want to make a huge PR, instead I'll do several small ones to make it
bearable :).

That's why this may seem a little bit out of context.

I need this method because the undo.data file has lines with the following format:

time 1508839796
new [description:"Create ansible playbook to deploy backupninja" entry:"1482393062" est:"1.000000" modified:"1486571719" priority:"L" project:"sis" status:"pending" tags:"ansible" uuid:"22a0a917-923b-48a0-a20f
-f08e2eca119a"]
---

I need to know the available task attributes and udas to convert that format to
json to be able to load it into a dictionary. I saw this way of doing things better than accepting any word:" regexp

for key in self.backend.available_task_attrs:
	... process the data_line ...

history_entry = json.loads(data_line)

You can find the whole code
here
under the TaskHistory class, method _convert_history_entry

@lyz-code
Copy link
Contributor Author

lyz-code commented Mar 6, 2018

@robgolding Seems that the SCM of travis is broken again

@robgolding
Copy link
Collaborator

I'd prefer to see all the changes in context @lyz-code, so one big PR is probably preferable here actually.

Also I need to fix the Git repo location now taskwarrior has moved to GitHub!

@lyz-code
Copy link
Contributor Author

lyz-code commented Sep 7, 2020

Closing for inactivity

@lyz-code lyz-code closed this Sep 7, 2020
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.

4 participants