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

User Guide has incorrect example code comments in "unicode-bom" rule page #11937

Closed
BrandonYeager opened this issue Jul 3, 2019 · 6 comments

Comments

@BrandonYeager
Copy link
Contributor

commented Jul 3, 2019

The example code in the page for the unicode-bom rule has incorrect comments (https://eslint.org/docs/rules/unicode-bom#require-or-disallow-the-unicode-byte-order-mark-bom-unicode-bom).

The "always" examples should say /*eslint unicode-bom: ["error", "always"]*/
The "never" examples should say /*eslint unicode-bom: "error"*/


See screenshot for a clearer explanation...

unicode-bom_UserGuideErrors

@platinumazure

This comment has been minimized.

Copy link
Member

commented Jul 3, 2019

I'm okay with fixing the first two examples, but I'm not sure we should change the latter 2 examples. I like being explicit with what option is being discussed. If we want to add another section showing the default behavior, I'd be okay with that.

I don't feel strongly about this, though.

BrandonYeager added a commit to BrandonYeager/eslint that referenced this issue Jul 3, 2019

@BrandonYeager

This comment has been minimized.

Copy link
Contributor Author

commented Jul 3, 2019

I submitted a PR based on my original proposed change. I agree that being explicit would be better based on the above comment.

I haven't contributed to many open source projects, so I have a question... Would it be preferable to modify my existing commit on my branch or adding a second commit modifying the code I currently have? (edit: it has already been reviewed)

@platinumazure

This comment has been minimized.

Copy link
Member

commented Jul 3, 2019

Hi @BrandonYeager, you can push a second commit to the branch-- we'll squash when we merge. This allows us to keep track of the full history even as new commits are added. Thanks!

@BrandonYeager

This comment has been minimized.

Copy link
Contributor Author

commented Jul 3, 2019

@platinumazure Thanks for the information! I'm looking forward to contributing more in the future!

One more question... when modifying a PR, do you still follow the code conventions for every commit message with the Tag and issue number? Or do you typically squash to the first commit message so it doesn't matter after the first message?

BrandonYeager added a commit to BrandonYeager/eslint that referenced this issue Jul 3, 2019

@platinumazure

This comment has been minimized.

Copy link
Member

commented Jul 3, 2019

@BrandonYeager Our commit message check looks at the PR title if there are 2 or more commits in the PR, because that's what the suggested commit message is when we squash/merge. (If there is 1 commit, the first commit's message is used instead, and our commit message check looks at the commit message for that reason.) So you can put whatever you want in the later commit messages.

@BrandonYeager

This comment has been minimized.

Copy link
Contributor Author

commented Jul 3, 2019

@platinumazure Thank you for the information and the rapid responses to my questions! You guys are doing an awesome job!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.