Skip to content
This repository was archived by the owner on Dec 29, 2022. It is now read-only.

Prepare for 2018 edition #943

Merged
merged 9 commits into from
Jul 17, 2018
Merged

Conversation

Xanewok
Copy link
Member

@Xanewok Xanewok commented Jul 14, 2018

While dyn fixing annotations was straightforward, doing that for urneachable_pub was awkward... Not sure I agree with most of the changes - IMO pub(crate) adds more noise and regular pub should be treated as such, especially for bin targets, but that may be just me. Tried to go for the most restrictive visibility for the warned against items (not sure if that's good).

r? @nrc

Copy link
Member

@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.

Thanks for doing this! I think I would change all the pub(super) to pub(crate), and you can use crate rather than pub(crate) to make it a bit less noisey.

Do we need to opt-in to the edition in Cargo.toml?

@Xanewok Xanewok force-pushed the prepare-2018-edition branch from 8806132 to 2ef14af Compare July 16, 2018 16:44
@Xanewok
Copy link
Member Author

Xanewok commented Jul 16, 2018

Used crate and now the edition is in the Cargo.toml, thanks!

Did use official Edition transition guide (+ added #![warn(rust_2018_idioms)]) and rustfix for this time around.

So the experience with using cargo fix was somewhat good, although it did just mostly slap crate:: to imports.
The main problems I had:

  1. No obvious way to use it for other targets (i.e. tests) and --cfged code (Used it manually in the end)
  2. It just automatically applies rustc suggestions, which in this case wasn't enough - here we extern crate rls_span as span in main.rs file and use span; in other, but it couldn't handle these reexported extern crates.

For extern rls_* crates I didn't pub use them in main to be later reused as crate::<name> elsewhere, since I believe making the distinction makes it easier to build a mental model of the RLS and helps discovery of those external crates (I know I was confused at the beginning).

@nrc
Copy link
Member

nrc commented Jul 17, 2018

No obvious way to use it for other targets (i.e. tests) and --cfged code (Used it manually in the end)

--all-targets should work for this.

Thanks for the feedback!

@nrc nrc merged commit ae5e4f7 into rust-lang:master Jul 17, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants