Support deriving Collection

[ModProg]

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 #143 (comment).

closes #138

[ecton]

This is looking great so far. Thank you for adding trybuild -- the existing macros didn't have ui tests, with that added I'll be able to go back and get coverage over them.

[ModProg]

Also, not sure if we can somehow get the procmacros to be imported with the trait like the serde ones. Not sure what they are doing, tho. I think for clap 3.0 you also don't need to import the derive macro separately from the traits. Maybe it's really simple and I just overlooked it :D

[ecton]

Hah, so I just discovered that the other proc macro in that crate is unused. I moved it to actionable.

You should be able to put pub use bonsaidb_macros::Collection inside of core/schema/collection.rs, and that should make it work? I've only done it at the top-level of a crate before, but I have to believe that will work in a module too.

I should have also asked -- are you wanting to get this merged as-is, and come back to implement the views argument later? Happy to do a partial merge to get this baseline functionality in if that's what you were aiming for.

[ModProg]

Hah, so I just discovered that the other proc macro in that crate is unused. I moved it to actionable.

Should I just remove it in this PR?

[ModProg]

@ecton I'm not able to request a review, but I think it should be done implementation wise.

[ecton]

@ecton I'm not able to request a review, but I think it should be done implementation wise.

It was probably because I never actually "responded" to the original request. I actually did the action this time, so hopefully the flow works correctly next time -- still getting used to the github flow.

This looks great! I've added a comment about the one TODO left.

[ecton]

Should I just remove it in this PR?

Sure, sorry for missing this in the first pass through. If you didn't, I was just going to do it post-merge.

[ModProg]

I think the macros should work now :D

Feel free to add some testcases that make more sense. Exspecially for the view derive I was not sure.

[ecton]

Looks really great! The attribute-macro crate you've written looks really nice.

I only noticed one feature I failed to think about until this review.

[ecton]

If you'd like to do one more request (pushed 8d84ac1), authority can be optional in the macro, and the generated code should just call CollectionName::private("name") instead of CollectionName::new. If you'd prefer a new issue, happy to split.

[ecton]

Looks great, kick this back for review one last time once you've released the new attribute crate (and optionally add support for optional authority).

[ModProg]

If you'd like to do one more request (pushed 8d84ac1), authority can be optional in the macro, and the generated code should just call CollectionName::private("name") instead of CollectionName::new. If you'd prefer a new issue, happy to split.

I was thinking about this. Should here maybe be a way to disable this automatic behaviour, like a custom deny(no_authority) but I don't know if this is possible other than doing a feature deny-no-authority. Just a thought, not sure if it actually makes sense.

[ecton]

I was thinking about this. Should here maybe be a way to disable this automatic behaviour, like a custom deny(no_authority) but I don't know if this is possible other than doing a feature deny-no-authority. Just a thought, not sure if it actually makes sense.

It totally makes sense. I don't think it's strictly necessary -- a CollectionAlreadyDefined error will be returned as soon as trying to use a Schema that has duplicate names, so the user is already protected from themselves. The namespacing system is to try to prevent those runtime errors, but after watching the live coding session we watched, I realized:

• Today, there are no third party collections except those defined in bonsaidb::core::admin. Those are namespaced, so they won't conflict if you use only use private names.
• As long as third party authors read the documentation (unlike the person we were watching lol), they will realize they shouldn't release private collection names.

The last concern I have is that feature flags are additive -- if you can figure out a way to allow a single crate to "deny private names" without infecting all other crates, I'd love to see that feature. But I'm worried that a third party library import could all of a sudden break your project because it enabled the deny-private-names feature.

[ModProg]

Totally makes sense. This is also something that can be easily added after the fact.

The last concern I have is that feature flags are additive -- if you can figure out a way to allow a single crate to "deny private names" without infecting all other crates, I'd love to see that feature. But I'm worried that a third party library import could all of a sudden break your project because it enabled the deny-private-names feature.

Maybe I forgot how the features for crates work. I thought this would be fine:

• A enables the error in their bonsaidb include usage.
• B uses A and also includes bonsaidb without the error (and uses the macro from here.).

But I now remember there was something about using a crate with different features as rust obviously cannot figure out that this feature only effects proc_macro expansion and not actually bonsaidb_core. It might make sense to shy away from this. Would be great if rust would enable something like custom linting for procmacros.

[ecton]

Looks great, just noticed some version numbers update that shouldn't have been, and decided it was a good opportunity to try the Sugguest Changes feature in GitHub.

[ModProg]

Looks great, just noticed some version numbers update that shouldn't have been, and decided it was a good opportunity to try the Sugguest Changes feature in GitHub.

Let's hope it worked :D