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 missing attribute for struct fields #39

Merged
merged 1 commit into from
Sep 27, 2023

Conversation

teohhanhui
Copy link
Contributor

@teohhanhui teohhanhui commented Sep 4, 2023

Closes #32

@teohhanhui teohhanhui force-pushed the feat/missing-property-attribute branch from af3f4a3 to 9c38c99 Compare September 4, 2023 07:58
Copy link
Collaborator

@alexjg alexjg left a comment

Choose a reason for hiding this comment

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

Left a few comments. Regarding testing, I prefer an end-to-end test where you define a type which uses the attribute in question and then check that it behaves the way we expect when hydrating. Something like:

#[derive(Debug, Hydrate)]
struct MaybeString {
    #[autosurgeon(missng=std::default::Default)]
    value: Option<String>
}

#[test]
fn hydrate_missing() {
    let mut doc = automerge::AutoCommit::new();
    let value: MaybeString = hydrate(&doc).unwrap();
    assert!(value.is_none());
}

@@ -261,6 +261,25 @@ fn gen_newtype_struct_wrapper(

let inner_ty = quote_spanned!(field.span()=> #inner_ty);

let missing = if let Some(missing_fn) = attrs.missing() {
quote! {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think we'll want to use quote_spanned here to make sure that the compile error produced is comprehensible. It may be worth adding a trybuild based UI test to test the errors as described in https://ferrous-systems.com/blog/testing-proc-macros/#diagnostics-error-paths (I didn't do that previously because this was my first major procedural macro project).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@alexjg
Copy link
Collaborator

alexjg commented Sep 4, 2023

What do you mean by "do anything about the key"?

I don't think we need support for enums (provided we support variant fields).

@teohhanhui
Copy link
Contributor Author

teohhanhui commented Sep 4, 2023

What do you mean by "do anything about the key"?

As in, migrating the key, from no key to some key. But perhaps that's not a possible scenario and I'm simply mistaken.

EDIT: Now that I think about it, that doesn't seem like something a default would help with anyway...

I don't think we need support for enums (provided we support variant fields).

Okay, so I'll have to add support for variant fields at least. 🙈

@teohhanhui teohhanhui force-pushed the feat/missing-property-attribute branch 2 times, most recently from 68eda70 to a0a6dcf Compare September 5, 2023 18:32
@teohhanhui teohhanhui force-pushed the feat/missing-property-attribute branch 3 times, most recently from a0ace88 to 21b8444 Compare September 6, 2023 11:03
@teohhanhui
Copy link
Contributor Author

teohhanhui commented Sep 6, 2023

#[derive(Clone, Debug, PartialEq, Eq)]
struct Inner(u64);

#[derive(Clone, Debug, PartialEq, Eq, Hydrate)]
#[autosurgeon(hydrate = "hydrate_outer", missing = "Default::default")]
struct Outer(Inner);

This should be a parse error, but isn't... I'm not sure how to fix this without a refactoring of the parsing code.

Apparently we skip everything if there's hydrate_with at the container level:

if let Some(hydrate_with) = container_attrs.hydrate_with() {
return proc_macro::TokenStream::from(on_hydrate_with(&input, &hydrate_with));
}

@teohhanhui teohhanhui force-pushed the feat/missing-property-attribute branch from 21b8444 to e54d195 Compare September 6, 2023 16:08
@teohhanhui teohhanhui marked this pull request as ready for review September 6, 2023 16:10
@teohhanhui teohhanhui mentioned this pull request Sep 6, 2023
@teohhanhui teohhanhui force-pushed the feat/missing-property-attribute branch from e54d195 to 3b28455 Compare September 14, 2023 15:23
@teohhanhui
Copy link
Contributor Author

ping @alexjg

@alexjg
Copy link
Collaborator

alexjg commented Sep 21, 2023

Apologies, I missed this question. Will take a look tomorrow, maybe we will need to refactor a little. To be clear, the impact right now is that if you have a hydrate attribute the missing attribute is ignored?

@teohhanhui
Copy link
Contributor Author

teohhanhui commented Sep 22, 2023

To be clear, the impact right now is that if you have a hydrate attribute the missing attribute is ignored?

Only on the container, but we already ignore everything else if hydrate (or with) is present on the container.

Currently that would not produce parse errors, so it might be a bit confusing for the user... Their attributes are just silently ignored. Perhaps we could fix that in another PR?

@alexjg alexjg merged commit d2205b2 into automerge:main Sep 27, 2023
8 checks passed
@alexjg
Copy link
Collaborator

alexjg commented Sep 27, 2023

Yeah let's fix the parse errors in another PR

@alexjg
Copy link
Collaborator

alexjg commented Sep 27, 2023

This is now published as 0.8.2. Thanks @teohhanhui !

@teohhanhui
Copy link
Contributor Author

teohhanhui commented Sep 27, 2023

I think you forgot to push the v0.8.2 tag?

@teohhanhui teohhanhui deleted the feat/missing-property-attribute branch September 27, 2023 09:12
@alexjg
Copy link
Collaborator

alexjg commented Sep 27, 2023

Good spot, done.

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.

Optional or Default Hydration
2 participants