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

Restructure config #28

Merged
merged 10 commits into from
Apr 28, 2024
Merged

Restructure config #28

merged 10 commits into from
Apr 28, 2024

Conversation

DanielVoogsgerd
Copy link
Contributor

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.

 - Move account to array of table of accounts
 - Make icons configurable
@codecov-commenter
Copy link

codecov-commenter commented Dec 17, 2023

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%> (ø)

@jonhoo
Copy link
Owner

jonhoo commented Jan 3, 2024

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 😅

See https://docs.rs/toml/latest/toml/#deserialization-and-serialization for a reference.

@DanielVoogsgerd
Copy link
Contributor Author

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.

@DanielVoogsgerd
Copy link
Contributor Author

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

Copy link
Owner

@jonhoo jonhoo left a comment

Choose a reason for hiding this comment

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

This is looking pretty good! I left a couple of notes. No rush to get to them though :)

src/main.rs Outdated Show resolved Hide resolved
src/main.rs Outdated Show resolved Hide resolved
src/main.rs Outdated Show resolved Hide resolved
src/main.rs Outdated Show resolved Hide resolved
src/main.rs Outdated Show resolved Hide resolved
src/main.rs Outdated Show resolved Hide resolved
src/main.rs Show resolved Hide resolved
src/main.rs Outdated Show resolved Hide resolved
src/tray_icon.rs Outdated Show resolved Hide resolved
src/tray_icon.rs Outdated Show resolved Hide resolved
@DanielVoogsgerd
Copy link
Contributor Author

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.

src/main.rs Outdated Show resolved Hide resolved
Copy link
Owner

@jonhoo jonhoo 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!

It also looks as though old configuration files should still work just fine with this new setup — if so, we don't even need to do a new major version I don't think?

@DanielVoogsgerd
Copy link
Contributor Author

DanielVoogsgerd commented Feb 25, 2024

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.

@jonhoo
Copy link
Owner

jonhoo commented Mar 2, 2024

Good call!

And yes, I think you're right we should then just have folders 👍

BREAKING-CHANGE: This removes the legacy option folder. From now on use
folders instead.
@DanielVoogsgerd
Copy link
Contributor Author

I'll try to push a change later today.

Yeah, that did not happen 😅

But, I think it should be correct now. folder has been removed and README.md has been updated.

@jonhoo jonhoo merged commit 7a342ea into jonhoo:main Apr 28, 2024
9 of 11 checks passed
@jonhoo
Copy link
Owner

jonhoo commented Apr 28, 2024

Perfect, thank you! Release coming shortly 👍

@jonhoo jonhoo mentioned this pull request Apr 28, 2024
jonhoo added a commit that referenced this pull request Apr 28, 2024
This change includes few but notable changes:

- A significant upgrade to the `imap` crate.
- Significantly updated parsing for buzz config files (#28),
  which includes support for user-configured icons.
- Support for multiple folders (#24).
- A complete swap of the library used for tray icons (#25).

From a user's perspective, the main change is that config files need to
be updated to match the new format. In particular, this means changing
`accounts` from a set of named tables to an array of tables, and moving
from `folder = "something"` to `folders = ["something"]`. See the README
for an updated example config.

Big thanks to @DanielVoogsgerd for the changes in this release!
@jonhoo
Copy link
Owner

jonhoo commented Apr 28, 2024

Released as 2.0.0 🎉

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.

None yet

3 participants