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

Implement style_edition config #5787

Closed
wants to merge 1 commit into from
Closed

Conversation

ytmimi
Copy link
Contributor

@ytmimi ytmimi commented Jun 19, 2023

This PR adds the style_edition configuration value and the StyleEditionDefault trait and implements the trait for all current configuration options.

This new trait allows each configuration option to specify its default value for different style editions. Before style edition there was no mechanism that allowed changing a configs default value.

A new #[style_edition] procedural macro is added to make implementing style edition much easier. Admittedly I haven't worked with syn and quote all that much so any advice on simplifying the procedural macro code is welcome!

@ytmimi
Copy link
Contributor Author

ytmimi commented Jun 19, 2023

Also want to highlight that I think I'll need to add additional tests to ensure that style edition is set correctly in different scenarios before this gets merged. Mostly leaving this as a note for myself so I don't forget to add those tests

@calebcartwright
Copy link
Member

Whew, this one looks like it'll be fun to review. I assume the changes proposed here are atomic, and that there's no way to decompose it in any form into two (or more) smaller PRs?

@ytmimi
Copy link
Contributor Author

ytmimi commented Jun 27, 2023

If it helps, I think we could break this up a little. All the proc macro code could be one PR, and then there might be some other changes that could be broken up into smaller PRs, but once we start messing with the configuration a lot of it needs to come together.

Let me know what you think would be best. I'm happy to make any changes.

@calebcartwright
Copy link
Member

If it helps, I think we could break this up a little. All the proc macro code could be one PR, and then there might be some other changes that could be broken up into smaller PRs, but once we start messing with the configuration a lot of it needs to come together.

Let me know what you think would be best. I'm happy to make any changes.

I think the typical adage about a higher quantity of smaller PRs being preferable to a lower quantity of larger PRs holds here as well. I trust and defer to your judgement around determining the best way to slice and dice the changes

@calebcartwright
Copy link
Member

Also, I really want to underscore my appreciation for your efforts on this front. It's obviously no small task, but the work is of paramount importance and I'm grateful you've already made this much progress.

I cannot over stress how significant and beneficial the style edition mechanism is going to be in addressing some incredibly long standing and thorny issues (I will also give yet another shout out to @joshtriplett for coming up with the style edition solution 🙏)

@ytmimi
Copy link
Contributor Author

ytmimi commented Apr 3, 2024

work for this is being done in other PRs

@ytmimi ytmimi closed this Apr 3, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants