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

Re-export ArrowSerialize, ArrowDeserialize and ArrowField at the top #71

Open
aldanor opened this issue Oct 8, 2022 · 4 comments
Open

Comments

@aldanor
Copy link

aldanor commented Oct 8, 2022

Let's be ergonomic, and more like serde.

There's only 3 modules, they are all tiny, essentially 1 main symbol in each, and having to always access arrow2_convert::serialize::ArrowSerialize is not really cool.

Also, having both ArrowField symbols (trait and proc-macro) in the same scope would be a more common way of doing things and a bit nicer.

Plus proc-macros (and regular macros) will generate a lot less code spam.

Suggestion: export all three main traits at the crate root. If that's ok, I can open a quick PR (and I'll update the proc-macros in the generics branch).

@ncpenke
Copy link
Collaborator

ncpenke commented Oct 8, 2022

@aldanor Thanks that makes sense. Can we follow a prelude approach? arrow2_convert::prelude::* would pull in all three.

@aldanor
Copy link
Author

aldanor commented Oct 8, 2022

I think prelude makes sense for bigger crates with dozens and dozens of symbols, where in most use cases you would import a lot of at once - it's actually quite a rare scenario.

I personally think it's a bad pattern for smaller crates with just a few symbols, and star-imports are best avoided if possible. Clippy even has a wildcard_imports lint, further indicating use prelude::* is a bit of an anti-pattern.

Whereas crates where there's just a few 'big' symbols like Serialize/Deserialize in serde typically just re-export them at the top (link) to simplify life for everyone, while keeping all the other types within their modules (e.g. ser and de in serde, thanks to them for keeping those module names short, too...).

Here's another good example (borsh crate): https://github.com/near/borsh-rs/blob/4f436f9aea92a420be3c5a62c17cf4e899b28451/borsh/src/lib.rs#L6-L17. It, again, follows the exact same structure as in serde:

  • ser and de public modules, holding all of the API
  • big symbols like Serialize / Deserialize re-exported at the top (shadowing their proc-macro versions, same as in serde)
  • no prelude; a few other major symbols also re-exported at the top

Here's another one (deser crate): https://github.com/mitsuhiko/deser/blob/main/deser/src/lib.rs. Again, same story - Serialize/Deserialize traits/proc-macro-traits at the top, ser + de modules.

And another one (bincode crate): https://github.com/bincode-org/bincode/blob/trunk/src/lib.rs. Same - decode/encode traits at top, ser + de public modules.

In fact, if I might suggest, renaming serialize -> ser and deserialize -> de along with the re-exports would make arrow2_convert more compliant with common crate structure. Would it be something to consider given we're below 1.0? (the existing serialize/deserialize modules can even be kept public and doc-hidden to avoid breakage)

Apologies for being so pedantic, I just wanted to help make this crate more conformant to the typical layout of crates of this type :)

@ncpenke
Copy link
Collaborator

ncpenke commented Oct 8, 2022

No problem, all good suggestions and makes sense to me. Please go for it!

@aldanor
Copy link
Author

aldanor commented Oct 8, 2022

Sounds good, I'll sketch a PR later :) (generics first though!)

@ncpenke ncpenke mentioned this issue Oct 8, 2022
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

No branches or pull requests

2 participants