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

Ignore unrelated field attributes #36

Merged
merged 2 commits into from
Apr 15, 2018
Merged

Ignore unrelated field attributes #36

merged 2 commits into from
Apr 15, 2018

Conversation

siler
Copy link
Contributor

@siler siler commented Apr 12, 2018

Currently all attributes on fields are parsed by derive-new. This change updates the attribute parsing logic to ignore any attribute that aren't specifically for derive-new. For example, previously:

#[derive(new)]
pub struct All {
    #[allow(missing_docs)]
    pub x: i32
}

Would result in the following error:

error: proc-macro derive panicked
--> tests\test.rs:289:10
|
289 | #[derive(new, PartialEq, Debug)]
| ^^^
|
= help: message: Invalid #[new] attribute: #[new(missing_docs)]

The revision of compiletest was updated to allow all tests to build on stable. It might be a good idea to configure the CI to use the minimum supported version of Rust to execute tests.

Additionally, I was having issues building tests due to the workspace configuration and syn::Generics. I didn't look into it much, but removed the workspace configuration so it wouldn't affect others.

Copy link
Owner

@nrc nrc left a comment

Choose a reason for hiding this comment

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

Looks good, just curious about the Cargo.toml change

Cargo.toml Outdated

[workspace]
members = ["testcrate"]
syn = "0.13"
Copy link
Owner

Choose a reason for hiding this comment

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

Why is this change necessary?

Copy link
Contributor Author

@siler siler Apr 13, 2018

Choose a reason for hiding this comment

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

When the workspace is set up I see the following:

derive-new\testcrate> cargo test
   Compiling libc v0.2.39
   Compiling kernel32-sys v0.2.2
   Compiling derive-new v0.5.3
   Compiling serde_derive v1.0.37
   Compiling term v0.4.6
error[E0599]: no method named `make_where_clause` found for type `syn::Generics` in the current scope
  --> github.com-1ecc6299db9ec823\serde_derive-1.0.37\src\bound.rs:47:14
   |
47 |     generics.make_where_clause()
   |              ^^^^^^^^^^^^^^^^^

I looked into it a bit but didn't root cause it. Might not occur on a *nix box if the build failed in term.

Copy link
Owner

Choose a reason for hiding this comment

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

This is an error in serde_derive, and it would be weird to see that only on some platforms. I'm on Mac OS and build without error. Maybe it is due to how you are running the tests? I run cargo test --all from the root directory, rather than anything in testcrate

Copy link
Contributor Author

@siler siler Apr 13, 2018

Choose a reason for hiding this comment

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

Yeah, that seems unlikely. I see the same errors when I execute tests that way.

Edit: happens with both stable and nightly msvc.

Copy link
Owner

Choose a reason for hiding this comment

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

ping @dtolnay any idea what might be going on with this Serde build error?

Copy link
Contributor

Choose a reason for hiding this comment

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

No idea but it seems like the sort of thing that cargo update (or not having a lockfile checked in) would fix.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Looks like you're right, added it back.

@siler
Copy link
Contributor Author

siler commented Apr 15, 2018

Well that is amusing.

@nrc nrc merged commit 2df488f into nrc:master Apr 15, 2018
@nrc
Copy link
Owner

nrc commented Apr 15, 2018

Thanks!

@nrc
Copy link
Owner

nrc commented Apr 15, 2018

Published 0.5.4

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.

3 participants