jonhoo
Restructure config
As promised, a restructured version of the config as mentioned in: #6.
I tried to keep fairly consistent with the current code structure and not refactor too much. However, I think it might not be the worst idea to refactor at least the configuration parsing, as this is becoming quite unwieldy.
This however will probably suffice for the major version bump. If you like to see any changes or if you have some tips, let me know, those are always welcome.
Codecov Report
Attention: Patch coverage is 0% with 124 lines in your changes are missing coverage. Please review.
Project coverage is 0.0%. Comparing base (
d932999) to head (41fe28d). Report is 7 commits behind head on main.
Additional details and impacted files
| Files | Coverage Δ | |
|---|---|---|
| src/tray_icon.rs | 0.0% <0.0%> (ø) |
|
| src/main.rs | 0.0% <0.0%> (ø) |
Oh, sorry I wasn't clear — I think we should move to #[derive(Deserialize)] for an actual type (or set of types). That should make all of this much less unwieldy :sweat_smile:
See https://docs.rs/toml/latest/toml/#deserialization-and-serialization for a reference.
No worries, I tried to stay consistent with the current codebase to get the changes as small as possible, but I will gladly move the deserialization to serde.
That took longer than I had hoped, but I finally found some time to do the refactor. Not my best work; there is a fair bit of cloning that can be removed by restructuring or reference counting, but I wanted to keep the changes semi in-scope. Feel free to nitpick, but I'm not sure how soon I can get around to fixing everything. Feedback is always welcome.
Thanks
Thanks for the detailed feedback, almost all the suggestions have been applied, only the removal of the clones is not really possible I think (see response on review). This could possibly be solved using references, but I'm not sure if that is worth the hassle.
I don't think that is correct. Where normally, we would define accounts as named tables. It has been replaced with an array of tables called account now.
That does remind me of the fact that we should update the README accordingly. I'll try to push a change later today.
Considering the support around the tray-icon has changed as well, I think a major bump is due.
One final thing to consider is that if you bump the version, do you want to continue to support both the folder and the folders configuration option. I did it like this to make it backwards compatible, but maybe it is best to drop folder right now, as people will have to adjust their configuration anyway.
Good call!
And yes, I think you're right we should then just have folders :+1:
I'll try to push a change later today.
Yeah, that did not happen :sweat_smile:
But, I think it should be correct now. folder has been removed and README.md has been updated.