Skip to content

Update strum to Rust edition 2018 #110

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

Merged
merged 3 commits into from
Sep 17, 2020
Merged

Update strum to Rust edition 2018 #110

merged 3 commits into from
Sep 17, 2020

Conversation

jplatte
Copy link
Contributor

@jplatte jplatte commented Sep 2, 2020

Fixes #109.

I've updated the docs too (removing the mentions of extern crate), is that okay?

@Peternator7
Copy link
Owner

Thanks for opening the PR :) It looks like the build failures are legit, and there's probably some missing usings in the test cases. I'll be good to merge this once we fix the build.

@jplatte
Copy link
Contributor Author

jplatte commented Sep 7, 2020

Huh, so really the way to solve this would be changing #[cfg(feature = "derive")] on the macro re-exports to #[cfg(any(feature = "derive", doctest))], but that doesn't work. Alternatively CI could only run tests with --all-features, but that seems like a pretty bad idea. WDYT?

I guess the doctests could also have

use strum_macros::Foo;
use strum::Foo as _;

but really in the real world everybody would write

use strum::Foo;

instead and activate the derive cargo feature.

@jplatte
Copy link
Contributor Author

jplatte commented Sep 14, 2020

@Peternator7 friendly ping

@Peternator7
Copy link
Owner

I was poking around with this locally, and it looks to me like it works if I call cargo test from the root of the repository (the workspace) but fails if it's run from any of the subprojects. Can you confirm whether that behavior works for you?

If that's the case, we might be able to just update the appveyor script to call cargo test from the repo root instead of needing to step in and out of each of the projects.

@jplatte
Copy link
Contributor Author

jplatte commented Sep 17, 2020

Yes, by calling cargo test from the workspace root the strum derive feature is enabled because it is required by another workspace member. cargo test --features derive or cargo test --all-features inside the strum directory should work too. Are your okay with doctests not working without that feature turned on?

@Peternator7
Copy link
Owner

So I think I'm okay with that. The only people really running the tests should be contributors to this repo, and I would expect the most natural thing for someone to do is build the workspace and that works as expected. I pushed some changes last night that fixed the build and started actively building 1.31 for appveyor.

The gates on this PR pass now so let me review the content, but we should be good wrt the build.

@Peternator7 Peternator7 merged commit b517f23 into Peternator7:master Sep 17, 2020
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.

Convert to Rust edition 2018?
2 participants