Skip to content

Option to not create a new line before await (await-same-line) #4888

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

Closed
wants to merge 3 commits into from

Conversation

kangalio
Copy link

@kangalio kangalio commented Jul 3, 2021

I've taken a shot at implementing #4425 as a first-time contribution.

Tasks left to do:

  • Decide how to handle line and block comments in front of .await
  • Make tests run with await-same-line instead of default config somehow
  • Add documentation to Configurations.md

@calebcartwright calebcartwright changed the base branch from master to stbu July 30, 2021 02:37
@calebcartwright
Copy link
Member

Thank you so much for your work on this (as well as #4889), and apologies for the delay in providing feedback.

I'll lead with the bad news up front and note that I'm going to close both of these PRs, but want to explain why and the alternative direction we need to go, especially on the off chance you'd be interested in helping with that 😄

Fundamentally, the realization we've come to is that chains are too complicated with too much subjectivity to support the standard forced-rewriting-according-to-some-predefined-rules paradigm via configuration options that rustfmt uses.

As you're likely already aware, rustfmt is an AST-driven pretty printer style formatter that completely rewrites code based on the rules codified in the Rust Style Guide independently of how the code was originally formatted (with a handful of rare exceptions).

Configuration options are a great fit for enabling formatting use cases which diverge from the that Style Guide when those cases are bounded in scope (e.g. tabs or spaces, opening braces on the same line or the next line, etc). Grouping of chain items on the other hand is completely open ended and just about everyone has a different opinion on which items should be grouped/split, which in turn can vary by context. Attempting to tackle that with configuration options would quickly spiral out of control, both in terms of the configuration surface and codebase complexity, as well as making conflicting configurations a near certainty.

The alternative strategy with that config option based forced rewriting would be picking a couple of favorites and adding config options for those which would satisfy a small subset of use cases, but it would still add complexity to the code and fail to address the large swath of other use cases leaving users frustrated.

Neither of those variants are going to be viable, and in fairness we've probably got some issues that should be closed/updated to reflect that fact. Instead, the best path forward really seems to be to enable an opt-in alternative (rustfmt's defaults always have to adhere to the Style Guide) that would use the original/input formatting of the chains to inform how the chain items are grouped. This would give an option to everyone who feels strongly about needing their chains to be different than what the Style Guide prescribes, regardless of the specifics of their chain and desired groupings. There will almost certainly be some constraints (e.g. rustfmt wouldn't split the chain items even if it exceeds the max_width, or maybe the preservation of chain item group only works for single line chain items, etc.).

I've done enough thought experimentation that I feel this should be possible to do, but it's almost certainly a large chunk of work. Additionally, I suspect an existing change would be needed in the handling of chain items that exceed the max width.

Relevant issues #3863 and #4306

@kangalio
Copy link
Author

Thank you for the considerate and detailed explanation!

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.

2 participants