feat: added support for toml config #88

ErikBjare · 2020-04-19T16:30:27Z

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 · 2020-04-19T16:38:36Z

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

new alerts:

1 for Unused import

codecov · 2020-04-19T16:48:09Z

Codecov Report

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

@@            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%)`	⬇️
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 · 2020-04-19T16:53:20Z

This pull request introduces 1 alert when merging f19f7e0 into b2d9179 - view on LGTM.com

new alerts:

1 for Unused import

lgtm-com · 2020-04-19T17:09:01Z

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

new alerts:

1 for Unused import

lgtm-com · 2020-04-19T17:27:35Z

This pull request introduces 1 alert when merging f4b1bb8 into b2d9179 - view on LGTM.com

new alerts:

1 for Unused import

xylix · 2020-04-19T17:51:13Z

aw_core/config.py

+            config = f.read()
+        config_toml = tomlkit.parse(config)
+    else:
+        # TODO: If file doesn't exist, write with commented-out default config


This is already implemented below, right?

Ah yes, forgot to remove the TODO

xylix · 2020-04-19T17:53:33Z

aw_core/config.py

+
+def save_config_toml(appname: str, config: str) -> None:
+    # Check that passed config string is valid toml
+    assert tomlkit.parse(config)


Should we have a schema to test against here and/or in the tests?

Also does that assert result in a good runtime error?

Since each user (server, watchers) of the API decides itself what config options to use I'm not sure if a schema makes sense.

But maybe it could check against the default config or something to make sure things are of the same type etc? Sounds like a lot of work though, and it will probably work just fine and errors should be rare and clearly user-caused (since the user will know that they edited the config).

Would be nice with a schema, but considering the work needed I don't think it's worth it.
I don't think the configs are large enough as of now that making a schema would add much value, if we find in the future that we would like that we can add that.

xylix · 2020-04-19T17:54:36Z

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

ErikBjare · 2020-04-19T18:49:22Z

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 · 2020-04-20T11:11:18Z

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?

johan-bjareholt

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.

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 the files are written is the same, commented with a "#" on each line to show the default.

Regarding programming API there's no point in comparing for two reasons:

it's very hard to make the API similar as the rust toml library has very strict parsing while this does not, it's not really comparable
The aw-server config.rs is not really written as a library so the API is just whatever fits aw-server-rust use-cases right now

johan-bjareholt · 2020-04-25T10:43:47Z

aw_core/config.py

+    else:
+        # TODO: If file doesn't exist, write with commented-out default config
+        with open(config_file_path, "w") as f:
+            f.write(_comment_out_toml(default_config))


This is the same way that aw-server-rust does it.
Whether it's the best solution though I'm not sure as the default config might become outdated if it gets updated in the future. It's a decent solution anyway. The other possible solutions I have in mind are neither better or worse just different, so might as well just be consistent with how aw-server-rust does it.

johan-bjareholt · 2020-04-25T10:50:56Z

aw_core/config.py

+
+def save_config_toml(appname: str, config: str) -> None:
+    # Check that passed config string is valid toml
+    assert tomlkit.parse(config)


Would be nice with a schema, but considering the work needed I don't think it's worth it.
I don't think the configs are large enough as of now that making a schema would add much value, if we find in the future that we would like that we can add that.

ErikBjare · 2021-06-15T10:29:43Z

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, ActivityWatch/aw-watcher-window#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.

aw_core/config.py

lgtm-com · 2021-06-15T10:41:57Z

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

new alerts:

1 for Unused import

ErikBjare · 2021-06-15T10:48:43Z

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 · 2021-06-15T10:53:41Z

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

new alerts:

1 for Unused import

ErikBjare mentioned this pull request Apr 19, 2020

Improved module detection, configuration, and added support for non-standard modules ActivityWatch/aw-qt#56

Merged

3 tasks

ErikBjare force-pushed the dev/config-toml branch from 76443e0 to f19f7e0 Compare April 19, 2020 16:46

ErikBjare force-pushed the dev/config-toml branch from f19f7e0 to 2325b35 Compare April 19, 2020 17:03

ErikBjare requested review from johan-bjareholt and xylix April 19, 2020 17:03

feat: added support for toml config

f4b1bb8

ErikBjare force-pushed the dev/config-toml branch from 2325b35 to f4b1bb8 Compare April 19, 2020 17:20

xylix suggested changes Apr 19, 2020

View reviewed changes

johan-bjareholt reviewed Apr 25, 2020

View reviewed changes

ErikBjare commented Jun 15, 2021

View reviewed changes

aw_core/config.py Outdated Show resolved Hide resolved

Merge branch 'master' into dev/config-toml

043cc3c

fix: removed resolved TODO

067bb69

ErikBjare merged commit 811712c into master Jun 15, 2021

ErikBjare deleted the dev/config-toml branch June 15, 2021 10:48

Uh oh!

feat: added support for toml config #88

feat: added support for toml config #88

Uh oh!

Conversation

ErikBjare commented Apr 19, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lgtm-com bot commented Apr 19, 2020

Uh oh!

codecov bot commented Apr 19, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

lgtm-com bot commented Apr 19, 2020

Uh oh!

lgtm-com bot commented Apr 19, 2020

Uh oh!

lgtm-com bot commented Apr 19, 2020

Uh oh!

xylix Apr 19, 2020

Choose a reason for hiding this comment

Uh oh!

ErikBjare Apr 19, 2020

Choose a reason for hiding this comment

Uh oh!

xylix Apr 19, 2020

Choose a reason for hiding this comment

Uh oh!

ErikBjare Apr 19, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

johan-bjareholt Apr 25, 2020

Choose a reason for hiding this comment

Uh oh!

xylix commented Apr 19, 2020

Uh oh!

ErikBjare commented Apr 19, 2020

Uh oh!

ErikBjare commented Apr 20, 2020

Uh oh!

johan-bjareholt left a comment

Choose a reason for hiding this comment

Uh oh!

johan-bjareholt Apr 25, 2020

Choose a reason for hiding this comment

Uh oh!

johan-bjareholt Apr 25, 2020

Choose a reason for hiding this comment

Uh oh!

ErikBjare commented Jun 15, 2021

Uh oh!

Uh oh!

lgtm-com bot commented Jun 15, 2021

Uh oh!

ErikBjare commented Jun 15, 2021

Uh oh!

lgtm-com bot commented Jun 15, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

ErikBjare commented Apr 19, 2020 •

edited

Loading

codecov bot commented Apr 19, 2020 •

edited

Loading

ErikBjare Apr 19, 2020 •

edited

Loading