Skip to content

Necessary commit to create summary of browser data if more than one browser bucket#70

Merged
ErikBjare merged 2 commits intoActivityWatch:masterfrom
nicolae-stroncea:allBrowserData
Nov 3, 2018
Merged

Necessary commit to create summary of browser data if more than one browser bucket#70
ErikBjare merged 2 commits intoActivityWatch:masterfrom
nicolae-stroncea:allBrowserData

Conversation

@nicolae-stroncea
Copy link
Copy Markdown
Member

No description provided.

@codecov
Copy link
Copy Markdown

codecov bot commented Aug 30, 2018

Codecov Report

Merging #70 into master will decrease coverage by 0.4%.
The diff coverage is 44.44%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #70      +/-   ##
==========================================
- Coverage   94.52%   94.12%   -0.41%     
==========================================
  Files          31       31              
  Lines        1352     1362      +10     
  Branches      218      219       +1     
==========================================
+ Hits         1278     1282       +4     
- Misses         37       42       +5     
- Partials       37       38       +1
Impacted Files Coverage Δ
aw_analysis/query2.py 99.01% <ø> (ø) ⬆️
aw_transform/__init__.py 100% <100%> (ø) ⬆️
aw_transform/sort_by.py 81.25% <33.33%> (-11.06%) ⬇️
aw_analysis/query2_functions.py 85.82% <40%> (-1.78%) ⬇️
aw_datastore/storages/peewee.py 95.74% <0%> (-0.69%) ⬇️
aw_core/__init__.py 100% <0%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c29d28a...3c4187e. Read the comment docs.

Copy link
Copy Markdown
Member

@johan-bjareholt johan-bjareholt left a comment

Choose a reason for hiding this comment

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

Please add a unit test as well, should be pretty easy.

return sum_durations(events)

@q2_function
def q2_sum_event_lists(events1: list, events2: list) -> List[Event]:
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

sum_event_list is probably a bad name, this is rather appending two lists. It's also generic enough to not have to be a event_list so that shouldn't be a part of the name either.

Maybe name it merge_lists or append_lists?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Or concatenate.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

It's not generic as it sorts by timestamp. If you give it an object without that property, it'll trigger an attribute error. That said, the name should probably include the fact that it returns a sorted list

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

In that case I wonder why does it do two things in one function? There's a seperate function for sorting by timestamp.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Seems I somehow missed the sort_by_timestamp. I'll separate it into two functions.

@johan-bjareholt
Copy link
Copy Markdown
Member

Is this in progress?
Would be very nice to have it added soon.

@nicolae-stroncea
Copy link
Copy Markdown
Member Author

I'm having some issues merging. I'll look into them once midterm season is over.

@ErikBjare
Copy link
Copy Markdown
Member

Fixed the issues, merging now.

@ErikBjare ErikBjare merged commit 389321c into ActivityWatch:master Nov 3, 2018
@ghost ghost removed the review label Nov 3, 2018
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.

3 participants