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

Add derive macro for Collection #138

Closed
ecton opened this issue Jan 20, 2022 · 8 comments · Fixed by #146
Closed

Add derive macro for Collection #138

ecton opened this issue Jan 20, 2022 · 8 comments · Fixed by #146
Labels
collections Issues impacting collections or views enhancement New feature or request good first issue Good for newcomers
Milestone

Comments

@ecton
Copy link
Member

ecton commented Jan 20, 2022

Collection currently has a lot of boilerplate code that could be reduced or eliminated by a derive macro. We should consider adding macros to help implement collections:

  • Collection Authority and Name can be set as parameters on the struct
  • DefaultSerialization can be auto-derived, or overridden with a parameter.
  • Views can be specified as a list of type names (e.g. [ByName, ...]).
  • We should consider Add Collection Lifecycles #69 and determine if there is any way to support deriving Collection while still offering the lifecycle event implementations easily.

Originally posted by @ModProg in #106 (comment)

Maybe other things could also be derived like DefaultSerialization.

Maybe also Collection, but that would only be sensible for once without views etc. but at least for my use that would be really comftable :D.

#[derive(Collection)]
#[collection_name = ["hoppi", "notification_subscription"]]
// or
#[collection_name = "hoppi.notification_subscription"]

If you want I would look into implementing the macro.

I have this a lot:

impl Collection for SubscriptionInfoTable {
    fn collection_name() -> Result<CollectionName, InvalidNameError> {
        CollectionName::new("hoppi", "notification_subscription")
    }

    fn define_views(_schema: &mut Schematic) -> Result<(), bonsaidb::core::Error> {
        Ok(())
    }
}
impl DefaultSerialization for SubscriptionInfoTable {}
@ecton ecton added collections Issues impacting collections or views enhancement New feature or request good first issue Good for newcomers labels Jan 20, 2022
@ecton ecton added this to To do in Khonsu Labs Roadmap via automation Jan 20, 2022
@ecton ecton added this to the v1.0 milestone Jan 20, 2022
@ModProg
Copy link
Collaborator

ModProg commented Jan 20, 2022

I could look into this as I said, but while I do know derive Macros, I have not a lot of experience with bonsaidb :)

@ecton
Copy link
Member Author

ecton commented Jan 20, 2022

I would love for you to try your hand at it if you're feeling motivated. My pre-coffee brain didn't see the offer for help.

I think I might prefer this syntax, but play around with some variations and see what you think feels most natural:

#[derive(Collection)]
#[collection(name = "Name", authority = "Authority", views = [a,b,c])]
struct MyCollection {...}

There's a crate bonsaidb-macros for these proc-macros to live within already. It could probably use some cleanup, as I believe it was my first set of proc macros ever.

If that syntax for views breaks rust-analyzer or you have a different idea on how to tackle that, feel free to suggest alternate ideas!

ModProg added a commit to ModProg/bonsaidb that referenced this issue Jan 22, 2022
I also added trybuild to test the error messages of derive macro.

The macro does not check for name validity as the restrictions will be
removed khonsulabs#143 (comment).

closes khonsulabs#138 partially
@ecton
Copy link
Member Author

ecton commented Jan 23, 2022

As per Discord

  • if you don't specify serialiation, implement DefaultSerialization
  • If serialization is None, do not implement any serialization trait
  • If you specify a typename, use the typename to implement this:
#[derive(Collection)]
#[collection(serialization = transmog_bincode::Bincode)]
struct Foo {}

impl SerializedCollection for Foo {
    type Contents = Foo;
    type Format = $arg;


    fn format() -> Self::Format {
        $arg::default()
    }
}

@ModProg
Copy link
Collaborator

ModProg commented Jan 24, 2022

Appart from the better syn support when using strict Meta attributes, this is rustfmt works better as well.

In my test it formatted the code using serialization(...) but not serialization = ....

@ModProg
Copy link
Collaborator

ModProg commented Jan 24, 2022

  • We should consider Add Collection Lifecycles Add Collection Lifecycles #69 and determine if there is any way to support deriving Collection while still offering the lifecycle event implementations easily.

This seams more like a future problem? If there is anything that can be done now for the macro let me know.

ModProg added a commit to ModProg/bonsaidb that referenced this issue Jan 24, 2022
I also added trybuild to test the error messages of derive macro.

The macro does not check for name validity as the restrictions will be
removed khonsulabs#143 (comment).

closes khonsulabs#138 partially
@ModProg
Copy link
Collaborator

ModProg commented Jan 24, 2022

There should probably be an example in the trait Collection docs right?

@ecton
Copy link
Member Author

ecton commented Jan 24, 2022

Yes, I'd love to have the Colleciton trait have two examples -- the derive example and the equivalent manual implementation.

I also think we should update all the examples to use the derive macro. I'm indifferent about whether unit tests should manually implement vs derive -- leaning towards derive.

These refactors can be handled in separate PRs.

@ecton
Copy link
Member Author

ecton commented Jan 24, 2022

This seams more like a future problem? If there is anything that can be done now for the macro let me know.

It is no longer relevant. Spent some time thinking about it and this will end up being like View -- a Collection and CollectionSchema trait eventually.

ModProg added a commit to ModProg/bonsaidb that referenced this issue Feb 4, 2022
I also added trybuild to test the error messages of derive macro.

The macro does not check for name validity as the restrictions will be
removed khonsulabs#143 (comment).

closes khonsulabs#138 partially
ModProg added a commit to ModProg/bonsaidb that referenced this issue Feb 6, 2022
I also added trybuild to test the error messages of derive macro.

The macro does not check for name validity as the restrictions will be
removed khonsulabs#143 (comment).

closes khonsulabs#138 partially
@ecton ecton closed this as completed in #146 Feb 6, 2022
Khonsu Labs Roadmap automation moved this from To do to Done Feb 6, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
collections Issues impacting collections or views enhancement New feature or request good first issue Good for newcomers
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

2 participants