Skip to content

Refactor/maccel_core::params::persist -> maccel_core::persist#77

Merged
Gnarus-G merged 6 commits intoGnarus-G:mainfrom
sy3c4ll:refactor/persist
Apr 21, 2025
Merged

Refactor/maccel_core::params::persist -> maccel_core::persist#77
Gnarus-G merged 6 commits intoGnarus-G:mainfrom
sy3c4ll:refactor/persist

Conversation

@sy3c4ll
Copy link
Copy Markdown
Contributor

@sy3c4ll sy3c4ll commented Apr 20, 2025

This PR includes BREAKING CHANGES. Though I doubt anyone but myself uses maccel_core separately.

As you may be aware, maccel_core::params::persist mainly houses definitions for ParamStore and SysFsStore, a trait for defining behaviour when retrieving and modifying parameters. However the module persist seems to bear no more similarity to params than, say, context, while params.rs is quite sizable and a split may be desirable.

At the same time, while I was using maccel_core in my sy3c4ll/maccel-gui I noticed quirks and inconsistencies with regards to persist::ParamStore, namely that while get and set took in self, get_current_accel_mode and set_current_accel_mode did not while also panicking on error instead of returning an anyhow::Result. This introduced difficulties where I couldn't handle mode switching errors, nor save the mode as a member variable in testing.

This PR addresses these issues.

  1. Separate pub mod persist in crates/core/src/params.rs into its own crates/core/src/persist.rs
  2. Remove mod validate in crates/core/src/params.rs and extract contents to params.rs while I'm at it, since the module housed only one function which was only necessary for a mysterious Display impl for Fpt in persist.rs...?
  3. BREAKING: Change function signatures and implementations of trait ParamStore in persist.rs as described

I've ensured it works for crates cli and tui already, there should be no problems there.

RSVP.

@vercel
Copy link
Copy Markdown

vercel Bot commented Apr 20, 2025

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
maccel ✅ Ready (Inspect) Visit Preview 💬 Add feedback Apr 21, 2025 5:41am

@Gnarus-G
Copy link
Copy Markdown
Owner

There are still some compilation errors currently, otherwise LGTM. When you fix those I'll merge.

@Gnarus-G Gnarus-G added enhancement New feature or request good first issue Good for newcomers labels Apr 21, 2025
@sy3c4ll
Copy link
Copy Markdown
Contributor Author

sy3c4ll commented Apr 21, 2025

Oops, looks like a last-minute addition slipped past my LSP. On it!

To document, my latest commit makes the following additional change:

  1. Change argument type of param in ParamStore::get from &Param to Param

@sy3c4ll
Copy link
Copy Markdown
Contributor Author

sy3c4ll commented Apr 21, 2025

All tests passed! I really skipped everything test-related when I was learning Rust, I shouldn't have done that. TIL.

Oh and uhh...

-    use maccel_core::{fixedptc::Fpt, ContextRef, Parameter};
+    use maccel_core::{ContextRef, Parameter, fixedptc::Fpt};

I may have inadvertently messed up a few import orders... I blame my auto-formatter. Should I revert all of these?

@Gnarus-G
Copy link
Copy Markdown
Owner

One more thing, remove that "BREAKING CHANGE" prefix from the commit message on BREAKING CHANGE: Refactor trait ParamStore for consistent error handling and ref self

Comment thread tui/src/utils.rs
@Gnarus-G
Copy link
Copy Markdown
Owner

All tests passed! I really skipped everything test-related when I was learning Rust, I shouldn't have done that. TIL.

Oh and uhh...

-    use maccel_core::{fixedptc::Fpt, ContextRef, Parameter};
+    use maccel_core::{ContextRef, Parameter, fixedptc::Fpt};

I may have inadvertently messed up a few import orders... I blame my auto-formatter. Should I revert all of these?

Order is not that important to me, what the formatter does is fine. I might add rustfmt configs at some point. But rn you can just format however your rustfmt does it.

@sy3c4ll
Copy link
Copy Markdown
Contributor Author

sy3c4ll commented Apr 21, 2025

The BREAKING CHANGE: is from Conventional Commits, but whatever you say is king. I'll rebase and force push in a jiffy.

@sy3c4ll
Copy link
Copy Markdown
Contributor Author

sy3c4ll commented Apr 21, 2025

Done.

@Gnarus-G
Copy link
Copy Markdown
Owner

The BREAKING CHANGE: is from Conventional Commits, but whatever you say is king. I'll rebase and force push in a jiffy.

Ain't nobody got time for all these different conventions. But it does look like they recommended putting that in the body/footer of the commit, not the subject line.

I'm generally down with putting whatever in the body.

@Gnarus-G Gnarus-G merged commit 2443c18 into Gnarus-G:main Apr 21, 2025
3 checks passed
@sy3c4ll
Copy link
Copy Markdown
Contributor Author

sy3c4ll commented Apr 21, 2025

Thank you thank you thank you! This happens to be my first successful PR, like, ever! This was such a pleasing experience, and I'm happy to be along in the ride.

@sy3c4ll sy3c4ll deleted the refactor/persist branch April 21, 2025 05:54
@Gnarus-G
Copy link
Copy Markdown
Owner

Thank you thank you thank you! This happens to be my first successful PR, like, ever! This was such a pleasing experience, and I'm happy to be along in the ride.

I'm very happy to be the one to merge your first ever PR.

sy3c4ll added a commit to sy3c4ll/maccel-gui that referenced this pull request Apr 21, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request good first issue Good for newcomers

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants