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 support for lists in query2 #50

Merged
merged 11 commits into from
May 25, 2018
Merged

Added support for lists in query2 #50

merged 11 commits into from
May 25, 2018

Conversation

johan-bjareholt
Copy link
Member

@johan-bjareholt johan-bjareholt commented Mar 27, 2018

  • List support
  • Misc query2 fixes

aw-webui needs these changes for compatibility ActivityWatch/aw-webui#66

@codecov-io
Copy link

codecov-io commented Apr 4, 2018

Codecov Report

Merging #50 into master will increase coverage by 0.34%.
The diff coverage is 96.96%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #50      +/-   ##
==========================================
+ Coverage   95.86%   96.21%   +0.34%     
==========================================
  Files          30       29       -1     
  Lines        1136     1188      +52     
  Branches      168      183      +15     
==========================================
+ Hits         1089     1143      +54     
+ Misses         22       19       -3     
- Partials       25       26       +1
Impacted Files Coverage Δ
aw_transform/__init__.py 100% <ø> (ø) ⬆️
aw_datastore/storages/mongodb.py 95.6% <100%> (ø) ⬆️
aw_datastore/storages/peewee.py 96.42% <100%> (+0.05%) ⬆️
aw_analysis/query2_functions.py 88.11% <86.66%> (+1.8%) ⬆️
aw_analysis/query2.py 99.01% <98.76%> (+0.58%) ⬆️

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 7aab2e7...e6ad952. Read the comment docs.

@ErikBjare
Copy link
Member

I think first and last are bad names for the functions. Consider that last(events, 50) would be equivalent to reversed(limit(reversed(events), 50).

@ErikBjare
Copy link
Member

Also, weird that AppVeyor fails. But doesn't seem to be caused by your commits.

@johan-bjareholt
Copy link
Member Author

I think first and last are bad names for the functions. Consider that last(events, 50) would be equivalent to reversed(limit(reversed(events), 50)

limit is even more confusing IMO because it doesn't specify what it's actually limiting.
Adding a reverse function however would be very useful!

@ErikBjare
Copy link
Member

Looks like me merging #55 created some conflicts, could you look into fixing them @johan-bjareholt and I'll implement reversed and limit (where limit would take the n first) instead of first and last.

@johan-bjareholt
Copy link
Member Author

Looks like me merging #55 created some conflicts

"Some" conflicts was an understatement...

Seems to work now atleast, but each individual commit is likely broken now so probably better to just sqash it all before merge later.

@johan-bjareholt
Copy link
Member Author

I think first and last are bad names for the functions.

Reverted to previous limit_events

@johan-bjareholt
Copy link
Member Author

Really odd that test_limit fails in travis, passes locally on my machine...

On travis bffa4d4 passes while 4663852 fails while on my machine it's the opposite.

Copy link
Member

@ErikBjare ErikBjare left a comment

Choose a reason for hiding this comment

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

Looks good to me, if you can fix the weird limit bug in CI.

Just a question, why remove the spaces around the = assignments in the queries? I find it more difficult to read that way.

@johan-bjareholt
Copy link
Member Author

Just a question, why remove the spaces around the = assignments in the queries? I find it more difficult to read that way.

Mistake during the rebase, will fix

@johan-bjareholt
Copy link
Member Author

Looks good to me, if you can fix the weird limit bug in CI.

Fixed

@johan-bjareholt
Copy link
Member Author

Improved coverage now a bit well too (99% on query2.py), merging

@johan-bjareholt johan-bjareholt merged commit 3689827 into master May 25, 2018
@ghost ghost removed the review label May 25, 2018
@johan-bjareholt johan-bjareholt deleted the dev/query2_list branch May 25, 2018 20:13
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