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

derive: Support #[row(crate = ...)] #189

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

YBoy-git
Copy link

@YBoy-git YBoy-git commented Dec 12, 2024

Summary

  • A quick fix for a small TODO in derive crate.
  • Add support for #[row(crate = ...)] attribute

@CLAassistant
Copy link

CLAassistant commented Dec 12, 2024

CLA assistant check
All committers have signed the CLA.

serprex
serprex previously approved these changes Dec 12, 2024
@YBoy-git YBoy-git changed the title derive: Use global scope in quote derive: Support #[row(crate = ...)] Dec 14, 2024
derive/src/lib.rs Outdated Show resolved Hide resolved
@serprex serprex requested a review from loyd December 14, 2024 17:23
serprex
serprex previously approved these changes Dec 14, 2024
Co-authored-by: Philip Dubé <serprex@users.noreply.github.com>
@@ -0,0 +1,2 @@
mod row;
pub use row::Row;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

pub(crate) (and all below). It seems this subcrate doesn't use https://github.com/ClickHouse/clickhouse-rs/blob/main/Cargo.toml#L17

@@ -35,12 +55,14 @@ fn column_names(data: &DataStruct, cx: &Ctxt, container: &Container) -> TokenStr

// TODO: support wrappers `Wrapper(Inner)` and `Wrapper<T>(T)`.
// TODO: support the `nested` attribute.
// TODO: support the `crate` attribute.
#[proc_macro_derive(Row)]
#[proc_macro_derive(Row, attributes(row))]
pub fn row(input: proc_macro::TokenStream) -> proc_macro::TokenStream {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This patch deserves a docstring

type Error = &'a syn::Attribute;

fn try_from(attr: &'a syn::Attribute) -> Result<Self, Self::Error> {
if attr.path().is_ident(ATTRIBUTE_NAME) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not very important (because it's a private API), but it's more straightforward to return an error (even a simple string) and panic in the From instance.


impl From<&[syn::Attribute]> for Attributes {
fn from(attrs: &[syn::Attribute]) -> Self {
let mut row = None;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is point to use None here instead of Row::default()?

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.

4 participants