Skip to content

fix: add rule to vscode settings #4881

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 3 commits into from
Apr 29, 2022
Merged

fix: add rule to vscode settings #4881

merged 3 commits into from
Apr 29, 2022

Conversation

Zamiell
Copy link
Contributor

@Zamiell Zamiell commented Apr 29, 2022

PR Checklist

There's no issue because this is a request from Josh.

Overview

The settings.json file contains VSCode settings that are specific to this repository/project.

Typically, in code repos, the IDE ruler is set to the same length as what the code style guide says and/or what the autoformatter is configured to.

For this repository, that value seems to be 80, because of the "printWidth": 80, specification in the ".prettierrc.json" file.

Thus, this project should explicitly represent that in the VSCode settings.

Reason

Having the IDE show a ruler is useful because the autoformatter (in this case Prettier) does not handle formatting comments.
For example, if I am writing a comment like this:

// This is a really long comment that has a lot of verbosity. It's going to explain exactly why I added all of this bullshit code below that at first glance seems unnecessary.
function foo() {}

Now I've broken the workflow for people who need the code to be at most 80 characters per line. In other words, for people reading the code, comments should be at most 80 characters for line for the same reasons that the code itself should be at most 80 characters per line.

Having the ruler on the screen is a helpful reminder to split up the comment like this:

// This is a really long comment that has a lot of verbosity. It's going to
// explain exactly why I added all of this bullshit code below that at first
// glance seems unnecessary.
function foo() {}

(Furthermore, without having the ruler, it's extremely difficult to "properly" split up the long line, since I don't know exactly where the breaks should go.)

Tangent 1

Is there an ESLint rule + autofixer for this? If not, there should be.

Tangent 2

Having the "printWidth": 80, value in the ".prettierrc.json" file is probably a bug, because 80 is already the default value for printWidth, and thus it doesn't have to be specified at all. It's easier to read a ".prettierrc.json" file if it only specifies changes to the default values. By having it specified at all, it implies that 80 is not the default for Prettier, which is confusing/misleading. I can also make changes to the ".prettierrc.json" file in this PR if you want.

@typescript-eslint
Copy link
Contributor

Thanks for the PR, @Zamiell!

typescript-eslint is a 100% community driven project, and we are incredibly grateful that you are contributing to that community.

The core maintainers work on this in their personal time, so please understand that it may not be possible for them to review your work immediately.

Thanks again!


🙏 Please, if you or your company is finding typescript-eslint valuable, help us sustain the project by sponsoring it transparently on https://opencollective.com/typescript-eslint. As a thank you, your profile/company logo will be added to our main README which receives thousands of unique visitors per day.

@nx-cloud
Copy link

nx-cloud bot commented Apr 29, 2022

☁️ Nx Cloud Report

CI is running/has finished running commands for commit d84b7b6. As they complete they will appear below. Click to see the status, the terminal output, and the build insights.

📂 See all runs for this branch


✅ Successfully ran 43 targets

Sent with 💌 from NxCloud.

@netlify
Copy link

netlify bot commented Apr 29, 2022

Deploy Preview for typescript-eslint ready!

Name Link
🔨 Latest commit d84b7b6
🔍 Latest deploy log https://app.netlify.com/sites/typescript-eslint/deploys/626be04ad098870008b24caa
😎 Deploy Preview https://deploy-preview-4881--typescript-eslint.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

Copy link
Member

@JoshuaKGoldberg JoshuaKGoldberg left a comment

Choose a reason for hiding this comment

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

Excellent PR description 😄 thanks!

@JoshuaKGoldberg JoshuaKGoldberg merged commit 3eab889 into typescript-eslint:main Apr 29, 2022
@Zamiell Zamiell deleted the vscode-settings branch April 29, 2022 13:45
@bradzacher
Copy link
Member

Having the "printWidth": 80, value in the ".prettierrc.json" file is probably a bug, because 80 is already the default value for printWidth, and thus it doesn't have to be specified at all. It's easier to read a ".prettierrc.json" file if it only specifies changes to the default values. By having it specified at all, it implies that 80 is not the default for Prettier, which is confusing/misleading.

We specify it because it makes it easy to see exactly what settings we are using. Also it means that if prettier changes any settings in a major, we will not be impacted by the change until we so chose it.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 30, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants