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

Mention options for correcting formatting issues when taking the "Quick fix" approach to contributing #3587

Conversation

swar8080
Copy link
Contributor

I followed the "Quick fixes" section of CONTRIBUTING.md and didn't realize a maximum line length is enforced by formatting rules. One of the maintainers suggested I use /fix:format but that doesn't work if the fork's branch name also exists in this repo. By default, github uses a branch name of patch-N when editing files from the browser, so a clash is likely

Hopefully calling this out will save contributors/reviewers time resolving formatting issues

@swar8080 swar8080 requested a review from a team November 22, 2023 14:05
@swar8080 swar8080 force-pushed the swar8080-contributing-quick-fix-formatting branch from be7069b to 91db7c1 Compare November 22, 2023 14:07
Copy link
Member

@svrnm svrnm left a comment

Choose a reason for hiding this comment

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

Minor comment, but let's have these changes eventually. Thanks for taking the time to write it down @swar8080

Comment on lines +84 to +87
- Commenting `/fix:format` on your pull request to trigger an automated script.
This requires a unique branch name, which can be edited under _View all
branches_ in your fork.

Copy link
Member

Choose a reason for hiding this comment

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

Let's add this in for now, but eventually I want to fix the automation script: we know what the problem is (if the branch name is used upstream as well the script gets "confused") and what the solution is (set the remote for the commit accordingly to the fork)

@cartermp cartermp merged commit 95110d9 into open-telemetry:main Nov 22, 2023
14 checks passed
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.

3 participants