Adding visibility option vis(...) to #[strum_discriminants] (#137)

[linclelinkpart5]

Closes #137

Validated using doctest example in the EnumDiscriminants section. While I was able to show in the doctest that I'm able to use the public discriminants properly (i.e. they work and are visible), I'm not sure how to show that I'm not exposing the original enum publicly. If anyone has suggestions for that, that would be helpful to include!

[linclelinkpart5]

Seems that Rust 1.31 isn't happy with using pub in the macro attributes, but the other Rust versions are. This would be outside the scope of my current knowledge of proc macros, is there a way this could be made to work with 1.31? And curious what Rust version this became legal?

[Peternator7]

Thanks for jumping in to help!!

I haven't seen an error like this before, but jumping around rust versions, it appears this behavior changed between 1.33 and 1.34, and it looks like the error is with rustc parsing the "pub" in the macro attribute as a keyword, and not actually related to the code being generated.

[linclelinkpart5]

If strict compatibility with Rust 1.31+ is needed, perhaps the vis keyword and logic could be put behind a feature gate?

[Peternator7]

I'm not too worried about compatibility because this is a new feature so it shouldn't break any existing code.

Let's do this:

• Try using vis(r#pub)) in the doc? I believe r#keyword let's you escape it, and that might fix the parse error in older versions of rust.
• If that works, let's add a comment above it that says on versions of rust 1.34 and greater, the r# is unnecessary.
• Can you add a test to the tests subproject? The docs get refactored more frequently than the tests so I'd like to make sure it's in a "permanent" spot. All's the test really needs to do is make sure vis(pub) and vis(pub(crate)) compile. Like you've said elsewhere. There's not a lot of functionality to validate.
• If getting vis(pub(crate)) to compile on 1.31 is too difficult, you can add a build cfg to the tests crate to skip that test. Here's the build.rs and here's an example of using the custom cfg.

[linclelinkpart5]

@Peternator7 Thanks! I tried out using "r#pub", and now it's the opposite: it only works on 1.31, and breaks the other Rust version builds. It seems I'd need to do a cfg of some sort for Rust <= 1.33 to use r#pub, and just pub otherwise.

[linclelinkpart5]

And to make things even trickier, it seems that even behind a #[cfg(bare_pub)] guard (where bare_pub is set for Rust >= 1.34), rustc 1.31 still sees the pub keyword and fails to compile. I'm not sure what to do here, it seems that nothing I'm trying is working. 😞

[Peternator7]

Quite the catch-22 isn't it 👎 I think I see a few options here:

1. Change the syntax to for visibility monitors to use a different word than pub. This seems unideal and requires more docs to explain the feature.
2. Up the MSRV to 1.34. This will probably happen eventually, but I don't think it's worth doing right now.
3. Split this feature into a separate code example and add a nocompile to the doc so it just doesn't get compiled.

I'm leaning towards option 3 because the other 2 seem unfeasible. It's not great that the doc won't be tested, but you've added test cases so we should still catch any regressions.

[Peternator7]

When I pulled your branch locally, I couldn't repro the bare_pub issue you were describing though so I'm less sure what the issue is there.

[linclelinkpart5]

@Peternator7 Of the three, I agree that no. 3 sounds the best. I'll proceed down that route, this would be a separate code block example right underneath the main one, correct?

When I pulled your branch locally, I couldn't repro the bare_pub issue you were describing though so I'm less sure what the issue is there.

I saw this error on travis, trying to build using Rust 1.31. I didn't try changing versions of my Rust toolchain locally, it was using Rust 1.47.

[linclelinkpart5]

Suggestion no. 3 worked great! Travis and AppVeyor look happy with the results. 😄

[Peternator7]

Looks great! Glad the builds are working. Only one comment. I think we can deduplicate some code.

[Peternator7]

Merged; thanks for the PR! I'll work on getting a release out this week.

[linclelinkpart5]

Awesome, thanks a bunch!

[Peternator7]

Strum 0.20.0 is published. https://crates.io/crates/strum