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 FromRow for structs with unnamed fields #985

Merged
merged 6 commits into from
May 7, 2024

Conversation

muzarski
Copy link
Contributor

Fixes: #852

Motivation

For some reason, users were not able to derive FromRow trait for structs with unnamed fields.
This PR fixes that.

Changes

Adjusted from_row_derive function from scylla-macros so that it supports structs with unnamed fields as well.

Pre-review checklist

  • I have split my patch into logically separate commits.
  • All commit messages clearly explain what they change and why.
  • I added relevant tests for new features and bug fixes.
  • All commits compile, pass static checks and pass test.
  • PR description sums up the changes and reasons why they should be introduced.
  • I have provided docstrings for the public items that I want to introduce.
  • I have adjusted the documentation in ./docs/source/.
  • I added appropriate Fixes: annotations to PR description.

@muzarski muzarski self-assigned this Apr 19, 2024
Copy link

github-actions bot commented Apr 19, 2024

cargo semver-checks found no API-breaking changes in this PR! 🎉🥳
Checked commit: 12a0436

@muzarski muzarski force-pushed the from_row_for_unnamed_structs branch from 6fb375b to 1826e69 Compare April 19, 2024 07:16
@muzarski muzarski requested review from Lorak-mmk and wprzytula April 19, 2024 07:30
@Lorak-mmk
Copy link
Collaborator

I thought that deserialization refactor will make this macro obsolete and later (before 1.0) we're going to remove it. What is the point of spending time on improvements to this macro?
@wprzytula

@wprzytula wprzytula added area/proc-macros Related to procedural macros enhancement New feature or request labels Apr 23, 2024
@wprzytula
Copy link
Collaborator

I thought that deserialization refactor will make this macro obsolete and later (before 1.0) we're going to remove it. What is the point of spending time on improvements to this macro? @wprzytula

That's a valid point. Once we have this PR ready, though, it's probably worthy to include it in the next release. 1.0 isn't going to be released very soon, so let's give users this enhancement as early as we can.

scylla-macros/src/parser.rs Outdated Show resolved Hide resolved
scylla-macros/src/parser.rs Show resolved Hide resolved
scylla-macros/src/parser.rs Outdated Show resolved Hide resolved
scylla-macros/src/from_row.rs Show resolved Hide resolved
scylla/src/macros.rs Show resolved Hide resolved
The docstring associated to this function was not easy to understand -
it got adjusted.
Extracted error message creation to closure, to not repeat
same multiple lines of code.
Add a parser util function that extracts the fields from a struct.
This commit prepares for code generation for unnamed fields.
The behaviour for named fields becomes unchanged, however code
generation logic was slightly adjusted so the very similar approach
can be applied to unnamed fields.
This commit adjusts the `from_row_derive` macro function that
implements `FromRow` for the structs, so that it works for
structs with unnamed fields as well.
@muzarski muzarski force-pushed the from_row_for_unnamed_structs branch from 1826e69 to 12a0436 Compare April 24, 2024 21:42
@muzarski muzarski requested a review from wprzytula April 25, 2024 11:40
}
});
crate::parser::StructFields::Unnamed(_fields) => todo!(),
Copy link
Collaborator

Choose a reason for hiding this comment

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

The approach here is better for readability that the previous one, but what I had in mind involved adding the Unnamed variant to the enum only in the next commit, together with the handling logic, so that todo!() and nonworking-yet-compiled code can be avoided at all.

@wprzytula wprzytula merged commit 729fa50 into scylladb:main May 7, 2024
11 checks passed
@Lorak-mmk Lorak-mmk mentioned this pull request May 9, 2024
@muzarski muzarski deleted the from_row_for_unnamed_structs branch October 29, 2024 14:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/proc-macros Related to procedural macros enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

FromRow is not working for type wrapper
3 participants