Mr custom #224
Mr custom #224
Conversation
Handle the tag hashtags in the raw_fact parsing.
|
Stefan Mertl seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account. You have signed the CLA already but the status is still pending? Let us recheck it. |
|
Hello Stefan, thank you for your interest and contributions. As I am finishing up a project at work this week I may not have time to have a look at your commits before next week - so this is just a initial response. btw: If you commit several distinct issues with your commits it would be beneficial to split them into separate feature branches/PRs. |
| @@ -366,9 +366,10 @@ def __init__(self, activity, start, end=None, pk=None, description=None, tags=No | |||
| self.start = start | |||
| self.end = end | |||
| self.description = description | |||
| self.tags = set() | |||
| self.tags = [] | |||
There was a problem hiding this comment.
Is there a specific reason for this change?
| if tags: | ||
| self.tags = set(tags) | ||
| tags = set(tags) | ||
| self.tags = [Tag(name = x) for x in tags] |
There was a problem hiding this comment.
Whist you are correct that this should be a set of Tag instances the mistake is already in L356! The constructor should expect an iterable of such Tag instances, not strings. I created an issue about this.
| @@ -468,7 +479,18 @@ def comma_split(string): | |||
| activity_name, back = at_split(rest) | |||
|
|
|||
| if back: | |||
| category_name, description = comma_split(back) | |||
| # Check for an existing hashtag. | |||
There was a problem hiding this comment.
This is an interesting approach. The next iteration of hamster-lib will however refactor the entire 'raw fact' parsing machinery in total, which is why no effort has been made to take care of tags.
| @@ -477,7 +499,7 @@ def comma_split(string): | |||
| category, description = None, None | |||
|
|
|||
| activity = Activity(activity_name, category=category) | |||
| return cls(activity, start, end=end, description=description) | |||
| return cls(activity, start, end=end, description=description, tags = tags) | |||
There was a problem hiding this comment.
there should not be kwarg assignment should not be surrounded by whitespace as per PEP8
There was a problem hiding this comment.
I understand the sentiment but I think for hamster-lib it makes more sense to provide one default export format that can easily be converted by other tools than trying to cover all possible variations.
In fact, there will be a "universal time tracker data conversion tool" called timetrackerimp you may want to check for timetrackerimp for exactly this purpose which have not release yet in the mid future.
|
Will close for now. Please let me know if you want to reopen/get back to this. |
I'm working on the hamster-cli and had to fix/extend some things in hamster-lib.