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 legacy-syntax and syn-2 features #22

Closed
wants to merge 1 commit into from
Closed

Add legacy-syntax and syn-2 features #22

wants to merge 1 commit into from

Conversation

smoelius
Copy link

@smoelius smoelius commented Nov 3, 2023

This PR does essentially two things:

  • Adds an enabled-by-default feature legacy-syntax and gates the test macro behind that feature
  • Adds a syn-2 feature to enabled use of syn version 2

gitoxide brings in two copies of syn, version 1 and version 2. Relying on maybe-async is the reason it needs version 1.

maybe-async cannot be easily converted to use syn version 2. maybe-async's test macro uses async as a nested attribute name. However, Rust 2018 added async as a keyword, and keywords cannot be used as attribute names. For this reason, syn does not parse them. One could try to work around the syn limitation, but one could also make a case that async should be changed to something else.

Despite all this, gitoxide does not use the test macro.

So, with the features described above, gitodixde can use maybe-async with default-features = false, features = ["syn-2"] and not have to bring in syn version 1.


I have not discussed this with the gitoxide maintainers. If you prefer, I could first get confirmation from them that they like this plan.

Also, I realize this is out of the blue. Feel free to nit anything, or close this PR if you don't like the idea.

@fMeow
Copy link
Owner

fMeow commented Dec 12, 2023

I think it fine to just dump the maybe_async::test attribute macro, since it is just a alias for a set of cfg_attr. We just need to clearly state how to use cfg_attr to add test in docs.

But unfortunately this introduces a breaking change in API, so I will release a minor version bump (v0.3).

@smoelius
Copy link
Author

We just need to clearly state how to use cfg_attr to add test in docs.

You would state that in CHANGELOG.md, or somewhere else?

@fMeow
Copy link
Owner

fMeow commented Dec 13, 2023

Both README.md, CHANGELOG.md and docs in rust code so that we can see it in docs.rs.

@smoelius
Copy link
Author

Just to be clear, the whole package gets upgraded to syn version 2, correct?

I..e, there should be no syn-1 feature to allow continued use of syn version 1, correct?

@mgeisler
Copy link

Hi @smoelius, thanks for working on this! (I'm just a random user.) I'm looking at importing this crate into Android and while we do have a copy of syn version 1, we would like to remove it in the future. So it would be great if maybe-async would no longer depend on the old version.

@smoelius
Copy link
Author

Thanks for your feedback, @mgeisler.

If this PR is merged, I expect it to be possible to use the crate without relying on syn version 1. I expect that independent of how @fMeow responds regarding a syn-1 feature,

@fMeow
Copy link
Owner

fMeow commented Jan 30, 2024

The PR #23 fully tackle the test macro problem. I will close this PR.

@fMeow fMeow closed this Jan 30, 2024
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