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

Add subcategories to reports #8

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Add subcategories to reports #8

wants to merge 3 commits into from

Conversation

mgedmin
Copy link
Member

@mgedmin mgedmin commented Jan 14, 2014

@@ -51,6 +51,19 @@ class Settings(object):
start_in_tray = False

report_style = 'plain'
"""If True then timestamp of the event will be present in categorized report"""
Copy link
Member Author

Choose a reason for hiding this comment

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

Please use comment syntax here.

@mgedmin
Copy link
Member Author

mgedmin commented Jan 14, 2014

Unfortunately GitHub doesn't show whitespace errors I saw with git show. Those need to be cleaned up (there should be no trailing whitespace and no tab characters in Python files).

This patch needs to be split into smaller bits, e.g. first split out the refactoring to pass the settings object to TimeLog/TimeWindow/Reports classes (but ditch the setter methods -- provide a mock class in the tests).

There are some bits missing:

  • changelog entry
  • documentation

I'm still uneasy about having the subcategory separator configurable.

@standagh
Copy link

Thank you very much for your time and comments! I agree with most of your points - formatting, PEP8,...

What do you think about whole concept of subcategories as implemented by this patch? Do you think is the feature OK and after solving technical details/errors can it be merged into master?

If so - I will seperate the patch to smaller bits.

Replace 4 tab characters with spaces
@mgedmin
Copy link
Member Author

mgedmin commented Jan 17, 2014

What do you think about whole concept of subcategories as implemented by this patch? Do you think is the feature OK and after solving technical details/errors can it be merged into master?

I haven't made up my mind completely about the syntax yet. Ideally the parsing will be contained in a single function/method, and we can experiment easily.

@standagh standagh mentioned this pull request May 14, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants