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

Upgrade syn/quote/proc-macro2 to 1.0 #164

Closed
tjkirch opened this issue Aug 27, 2019 · 5 comments · Fixed by #177
Closed

Upgrade syn/quote/proc-macro2 to 1.0 #164

tjkirch opened this issue Aug 27, 2019 · 5 comments · Fixed by #177
Assignees
Labels
breaking change Likely requires a SemVer version bump maintenance Keeping the wheels turning

Comments

@tjkirch
Copy link
Collaborator

tjkirch commented Aug 27, 2019

Assuming #154 decides to bump SNAFU's Rust requirement to 1.31, we'll be able to upgrade our syn, quote, and proc-macro2 dependencies to 1.0, which also require Rust 1.31.

https://github.com/dtolnay/syn/releases/tag/1.0.0

Some potential benefits mentioned in #145: "making more use of the powers given by the new versions, maybe mostly in quote. For example all of the repetition things mentioned are issues we encountered."

Potentially related, also from #145: "I've had a mild sense that snafu-derive's lib.rs is simply too big at this point and could benefit from some reorganization. Perhaps reviewing the code during such reorganization would be a good time."

@tjkirch tjkirch added maintenance Keeping the wheels turning breaking change Likely requires a SemVer version bump labels Aug 27, 2019
@tjkirch tjkirch self-assigned this Aug 27, 2019
@tjkirch
Copy link
Collaborator Author

tjkirch commented Aug 27, 2019

I've got a prototype of the dependency updates, Rust 1.31 minimum requirement, and Rust 2018 update in commits in: https://github.com/tjkirch/snafu/commits/upgrade-minimum

Certainly some of the changes in my commits are debatable, for example removing vs. replacing the rust_1_30 feature.

This does not include any improvements made available in quote 1.0, as shepmaster mentioned; hopefully I can work on that soon. It also does nothing to the organization of lib.rs. It may be worth splitting those into separate PRs, though?

@tjkirch
Copy link
Collaborator Author

tjkirch commented Sep 2, 2019

making more use of the powers given by the new versions, maybe mostly in quote. For example all of the repetition things mentioned are issues we encountered.

I looked through the quote 1.0 release notes, addressed what I could find, and pushed a couple more commits to https://github.com/tjkirch/snafu/commits/upgrade-minimum to make use of the improvements.

I initially thought that https://github.com/shepmaster/snafu/blob/master/snafu-derive/src/lib.rs#L1275 was a case of "Reusing values across repetitions" from the release notes, because changing f.name.clone to &f.name worked fine, but it turns out that worked with pre-1.0 quote too.

@shepmaster: I didn't find as much improvement as I expected to, which makes me think I missed some difficulties you had in the code. If you expect more than those two small improvements, I'd be interested in any pointers you could give!

@shepmaster: Also, if you have a moment for a quick look at the commits in that branch, I'd be interested in anything else you'd like to see covered in a PR, or if it seems ready for a PR now. You'll see I didn't touch lib.rs, which you mentioned as a possible change to include - I wasn't sure how it related to the other upgrades.

@shepmaster
Copy link
Owner

I think we might as well do this, so let's open up a PR to discuss specific changes. The two I know immediately are:

  • I'd reorder the commits: Drop 1.18, Drop 1.30, Upgrade to 2018, upgrade crates, various cleanups
  • Another cleanup is to replace let foo: &Vec<_> = &blah.collect() with let foo: Vec<_> = blah.collect().

@tjkirch
Copy link
Collaborator Author

tjkirch commented Sep 21, 2019

I just got back from a break, so I should be able to fix up what you suggested and get a PR out within a few days. :)

@tjkirch
Copy link
Collaborator Author

tjkirch commented Sep 24, 2019

I made the requested changes and opened #177.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking change Likely requires a SemVer version bump maintenance Keeping the wheels turning
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants