ActivityWatch / aw-core

Core library for ActivityWatch
Mozilla Public License 2.0
48 stars 47 forks source link

feat: added support for toml config #88

Closed ErikBjare closed 3 years ago

ErikBjare commented 4 years ago

Note: This deprecates the old config functions and leaves it up to users of the config API to migrate to the new functions themselves. It's possible there might be a better way.

lgtm-com[bot] commented 4 years ago

This pull request introduces 1 alert when merging 76443e0cbbbaed6d35e34d939c142987770400b0 into 5d7c04f1a2917b66bf827c909979ac1788633827 - view on LGTM.com

new alerts:

codecov[bot] commented 4 years ago

Codecov Report

Merging #88 (067bb69) into master (aa14c8b) will decrease coverage by 0.08%. The diff coverage is 95.12%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #88      +/-   ##
==========================================
- Coverage   90.84%   90.75%   -0.09%     
==========================================
  Files          34       34              
  Lines        1616     1655      +39     
  Branches      247      254       +7     
==========================================
+ Hits         1468     1502      +34     
- Misses        115      118       +3     
- Partials       33       35       +2     
Impacted Files Coverage Δ
aw_core/config.py 91.66% <95.00%> (-8.34%) :arrow_down:
aw_core/__about__.py 100.00% <100.00%> (ø)

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 aa14c8b...067bb69. Read the comment docs.

lgtm-com[bot] commented 4 years ago

This pull request introduces 1 alert when merging f19f7e0415542ef6ae4856f878c4de181e08e0f7 into b2d91796c202b3c4bc3db4d98acba323a9dab57d - view on LGTM.com

new alerts:

lgtm-com[bot] commented 4 years ago

This pull request introduces 1 alert when merging 2325b353153ba852c48ec50d875babb838f4b332 into b2d91796c202b3c4bc3db4d98acba323a9dab57d - view on LGTM.com

new alerts:

lgtm-com[bot] commented 4 years ago

This pull request introduces 1 alert when merging f4b1bb86446904e2221e64a9c2bff40d2e75f111 into b2d91796c202b3c4bc3db4d98acba323a9dab57d - view on LGTM.com

new alerts:

xylix commented 4 years ago

Looks good to me. Were you planning on implementing any sort of automatic migration from the old config to this new one?

ErikBjare commented 4 years ago

Not really, automatic migrations sounds like a lot of work.

I'm interested in getting this right though so if you have any suggestions for how the API could be improved or made more reliable please let me know.

Right now the save_config takes a str input, while the load_config returns a special dict (part of tomlkit) that maintains formatting.

ErikBjare commented 4 years ago

Now that I think about it, it would be good if we use a similar config API as in aw-server-rust, so might be worth comparing before merging.

How does this look to you @johan-bjareholt?

ErikBjare commented 3 years ago

Do we need this toml support for something? The built-in python .ini file was fine for our use-cases so I don't see the need for this, but I might be out of the loop on something.

The stdlib ConfigParser doesn't allow for writing comments to the config file, and since we write the file every time we load the config we have default configs that are impossible to distinguish from overrides, which makes it impossible to update defaults.

As an example, https://github.com/ActivityWatch/aw-watcher-window/pull/52 adds a new config option, and the first time the user runs aw-watcher-window with this new option, it gets written to the config file, which makes it impossible to change again in a future version (without disrespecting possible over-decided overrides).

That is why this PR only writes a commented out example config, which is the real motivation.

lgtm-com[bot] commented 3 years ago

This pull request introduces 1 alert when merging 043cc3cffb160e440dc534699adee5edaa7b51e5 into aa14c8bd843e35d0c65309d6b0827c62639f6551 - view on LGTM.com

new alerts:

ErikBjare commented 3 years ago

Merging this, needed for other stuff I'm working on.

Will mean a config-reset in v0.11, but we need to do that at some point anyway, might as well get it over with.

lgtm-com[bot] commented 3 years ago

This pull request introduces 1 alert when merging 067bb696f9c0b2b6facbd6a7ed13b2fe4c44e9c1 into aa14c8bd843e35d0c65309d6b0827c62639f6551 - view on LGTM.com

new alerts: