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.

Sign up for GitHub

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

Jump to bottom

Add style_edition 2027 #6324

Merged
merged 1 commit into from
Sep 27, 2024
Merged

Add style_edition 2027 #6324

merged 1 commit into from
Sep 27, 2024
+13 −1

Conversation

johnhuichen
Copy link
Contributor

No description provided.

johnhuichen
johnhuichen commented Sep 12, 2024
View reviewed changes
src/config/options.rs Outdated
@@ -458,6 +462,8 @@ impl From<Edition> for rustc_span::edition::Edition {
Edition::Edition2018 => Self::Edition2018,
Edition::Edition2021 => Self::Edition2021,
Edition::Edition2024 => Self::Edition2024,
// TODO: update to 2027 after rustc change
Edition::Edition2027 => Self::Edition2024,
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@calebcartwright rustc_span::edition::Edition doesn't have Edition2027 yet, how should we solve this problem

johnhuichen
johnhuichen commented Sep 12, 2024
View reviewed changes
src/config/style_edition.rs
@@ -30,7 +30,8 @@ macro_rules! style_edition_default {
$crate::config::StyleEdition::Edition2015
| $crate::config::StyleEdition::Edition2018
| $crate::config::StyleEdition::Edition2021 => $default_2015,
$crate::config::StyleEdition::Edition2024 => $default_2024,
$crate::config::StyleEdition::Edition2024
| $crate::config::StyleEdition::Edition2027 => $default_2024,
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Edition2024 default will also extend to 2027. Is this the desired behaviour?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For now I think that's fine since we don't currently have plans to change any default option values for 2027, but it would be nice to add a TODO comment explaining that we'll need to update the macro once we need to start changing defaults for 2027.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@johnhuichen want to remind you about the ask to add a comment here.

@ytmimi
Copy link
Contributor

ytmimi commented Sep 20, 2024

@johnhuichen Thanks for getting the ball rolling on this one. I think it would be best to keep this PR focused on just style_edition instead of also updating edition. As you noted, rustc hasn't provided a 2027 variant yet, but rustfmt will need an unstable StyleEdition::Edition2027 for future breaking formatting changes that aren't stabilized StyleEdition::Edition2024. Correct me if I'm wrong, but we only ever convert from Edition -> StyleEdition and not the other way around so I think that should be fine.

@johnhuichen
Copy link
Contributor Author

@johnhuichen Thanks for getting the ball rolling on this one. I think it would be best to keep this PR focused on just style_edition instead of also updating edition. As you noted, rustc hasn't provided a 2027 variant yet, but rustfmt will need an unstable StyleEdition::Edition2027 for future breaking formatting changes that aren't stabilized StyleEdition::Edition2024. Correct me if I'm wrong, but we only ever convert from Edition -> StyleEdition and not the other way around so I think that should be fine.

Sounds good, I can take another look

@johnhuichen
Copy link
Contributor Author

@ytmimi I am a little tied up for the next couple of weeks, and won't be able to make too much progress with rustfmt. Let me know if the delay is alright with you, or if it's better if I unassigned myself from the tasks

@ytmimi
Copy link
Contributor

ytmimi commented Sep 25, 2024

Thanks for the heads up! I don't think it should be an issue, but if the urgency around this changes we can open an alternative PR or build off of what you've got here.

@ytmimi
Copy link
Contributor

ytmimi commented Sep 25, 2024 •
edited
Loading

Also, just a small note: The team discourages merge commits so in the future please rebase your changes when getting your branch up to date.

@johnhuichen
Copy link
Contributor Author

johnhuichen commented Sep 25, 2024 •
edited
Loading

@ytmimi I removed Edition::Edition2027 everywhere. Let me know if there is any other code changes where I need to make to add StylEdition2027

fyi: I am keeping intermediate commits so it's easier for you to see what changes I made since last time. Before merging I will rebase everything to 1 commit

ytmimi
ytmimi reviewed Sep 25, 2024
View reviewed changes
Copy link
Contributor

@ytmimi ytmimi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Things looks good. Just want to add comments to the various places where 2027 defaults to 2024.

src/config/options.rs
@@ -523,6 +528,7 @@ impl From<StyleEdition> for rustc_span::edition::Edition {
StyleEdition::Edition2018 => Self::Edition2018,
StyleEdition::Edition2021 => Self::Edition2021,
StyleEdition::Edition2024 => Self::Edition2024,
StyleEdition::Edition2027 => Self::Edition2024,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's add a TODO comment here explaining that rustc_span::edition::Edition doesn't have an Edition2027 variant.

src/config/style_edition.rs
@@ -30,7 +30,8 @@ macro_rules! style_edition_default {
$crate::config::StyleEdition::Edition2015
| $crate::config::StyleEdition::Edition2018
| $crate::config::StyleEdition::Edition2021 => $default_2015,
$crate::config::StyleEdition::Edition2024 => $default_2024,
$crate::config::StyleEdition::Edition2024
| $crate::config::StyleEdition::Edition2027 => $default_2024,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@johnhuichen want to remind you about the ask to add a comment here.

@johnhuichen johnhuichen marked this pull request as ready for review September 26, 2024 20:27
@johnhuichen
Copy link
Contributor Author

Things looks good. Just want to add comments to the various places where 2027 defaults to 2024.

I just added comments as requested

@ytmimi ytmimi added release-notes Needs an associated changelog entry and removed pr-waiting-on-author labels Sep 27, 2024
@ytmimi ytmimi changed the title Add edition 2027 Add style_edition 2027 Sep 27, 2024
@ytmimi ytmimi merged commit 5f48fe9 into rust-lang:master Sep 27, 2024
26 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ytmimi ytmimi ytmimi approved these changes

Assignees
No one assigned
Labels
release-notes Needs an associated changelog entry
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@johnhuichen @ytmimi @rustbot