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

Split project into multiple crates #148

Merged
merged 8 commits into from
Apr 3, 2023
Merged

Split project into multiple crates #148

merged 8 commits into from
Apr 3, 2023

Conversation

gferon
Copy link
Collaborator

@gferon gferon commented Apr 2, 2023

This is a longstanding change that I kept pushing back.

Splitting presage into multiple crates bring the following benefits:

  • Allow adding store implementations without bringing all dependencies and using the big presage::Error struct
  • Lets us compile presage-cli separately and not as an example
  • Helps me test the public API better and tune the prelude imports

/cc @Schmiddiii

@Schmiddiii
Copy link
Contributor

A few notes:

  • I would put the README.md and LICENSE.md into the root of the repository and add smaller README.mds in all crates.
  • Why do we need the subdirectory crates? What about just putting all the crates in the root like libsignal-service-rs does? I would guess that depends on how many crates we would expect to have, but I think the root would probably be simpler.
  • Maybe instead of having a generic Box<dyn Error> for the store errors, it may be worth thinking about a type parameter for the Store that specifies the error and make the error generic over that. But that may also be overkill, so not sure about that one.

@gferon
Copy link
Collaborator Author

gferon commented Apr 3, 2023

I would put the README.md and LICENSE.md into the root of the repository and add smaller README.mds in all crates.

Just an oversight, done.

Why do we need the subdirectory crates?

Just a matter of preference, I moved it to the root for consistency.

Maybe instead of having a generic Box for the store errors, it may be worth thinking about a type parameter for the Store that specifies the error and make the error generic over that. But that may also be overkill, so not sure about that one.

If you look closely, this is already partially what's happening (the associated type inside of Store traits). Doing more than this would make the main presage::Error generic over the store error, which I don't think is a good idea because all type signatures returning it will have to carry around the type argument.

@Schmiddiii
Copy link
Contributor

Maybe the rustfmt.toml should also be in the root (I don't know if that then applies to all crates). Also the changelog should probably specify that this is a breaking change like you have shown in the README.md.

These are nothing you really have to solve though, just minor nitpicks. If you want, feel free to merge.

@gferon
Copy link
Collaborator Author

gferon commented Apr 3, 2023

It's not really a breaking change though, as the public API stays exactly the same. On the other hand, I expect actually breaking changes to be merged soon, so I bumped the version to 0.6-dev.

@gferon gferon merged commit 066e582 into main Apr 3, 2023
@gferon gferon deleted the split-crates branch April 3, 2023 11:12
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.

2 participants