Skip to content

[llvm][Docs] Explain how to handle excessive formatting changes #126239

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

Merged
merged 2 commits into from
Feb 10, 2025

Conversation

DavidSpickett
Copy link
Collaborator

Based on some feedback in Discord about a PR where a reviewer asked the author to move the formatting changes to a new PR, which appears to contradict the current form of this document.

I've added an explanation here, before the point where the author would be committing any of the formatting changes.

There are other ways this can go, for example some projects don't want the churn of formatting, or you can pre-emptively send a formatting PR, but I don't think enumerating them all here will help the audience for this text.

So I've recomended one path that will start them off well, and can branch off if the reviewers make requests.

Based on some feedback in Discord about a PR where a reviewer
asked the author to move the formatting changes to a new PR,
which appears to contradict the current form of this document.

I've added an explanation here, before the point where the author
would be committing any of the formatting changes.

There are other ways this can go, for example some projects don't
want the churn of formatting, or you can pre-emptively send a
formatting PR, but I don't think enumerating them all here will
help the audience for this text.

So I've recomended one path that will start them off well,
and can branch off if the reviewers make requests.
Copy link
Collaborator

@tru tru left a comment

Choose a reason for hiding this comment

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

Much clearer thanks!

Copy link
Collaborator

@preames preames left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@banach-space banach-space left a comment

Choose a reason for hiding this comment

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

LGTM % optional nits, thanks!

@DavidSpickett DavidSpickett merged commit f845497 into llvm:main Feb 10, 2025
6 of 8 checks passed
@DavidSpickett DavidSpickett deleted the commit-docs branch February 10, 2025 10:32
Icohedron pushed a commit to Icohedron/llvm-project that referenced this pull request Feb 11, 2025
…#126239)

Based on some feedback in Discord about a PR where a reviewer asked the
author to move the formatting changes to a new PR, which appears to
contradict the current form of this document.

I've added an explanation here, before the point where the author would
be committing any of the formatting changes.

There are other ways this can go, for example some projects don't want
the churn of formatting, or you can pre-emptively send a formatting PR,
but I don't think enumerating them all here will help the audience for
this text.

So I've recomended one path that will start them off well, and can
branch off if the reviewers make requests.
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.

4 participants