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

define the spaces/tabs policies and create checks. #1315

Closed
mvshmakov opened this issue May 19, 2020 · 9 comments
Closed

define the spaces/tabs policies and create checks. #1315

mvshmakov opened this issue May 19, 2020 · 9 comments
Labels
A: docs Area: user documentation (gatsby-theme-iterative) A: website Area: website help wanted Contributors especially welcome status: research Writing concrete steps for the issue ⌛ status: wait-response Waiting a response from the outside of the organization type: enhancement Something is not clear, small updates, improvement suggestions

Comments

@mvshmakov
Copy link
Contributor

There is a mixture of tabs/spaces in the repo. It would be better if there is only one unified standard for all of the codebase/documentation. In addition, it is a good practice to introduce some CI/pre-commit checks considering this convention.

The issue was revealed in this PR: #1305

@jorgeorpinel jorgeorpinel added dependencies Pull requests that update a dependency file (automatic) A: docs Area: user documentation (gatsby-theme-iterative) type: enhancement Something is not clear, small updates, improvement suggestions help wanted Contributors especially welcome labels May 19, 2020
@shcheklein
Copy link
Member

@mvshmakov what bothers me here is that we already have useTabs set to false:

  "tabWidth": 2,
  "useTabs": false,

in the https://github.com/iterative/dvc.org/blob/master/.prettierrc

and we do run it on a pre-commit hook that is installed when you run yarn.

I think the only place we allow spaces are code blocks. And probably prettier ignores them (and rightfully so). It feels there is some overlap between your editor settings and our .prettierrc. What editor do you use? Could you share the settings for it?

@shcheklein shcheklein changed the title Define the spaces/tabs policies and create checks. define the spaces/tabs policies and create checks. May 19, 2020
@shcheklein shcheklein added ⌛ status: wait-response Waiting a response from the outside of the organization status: research Writing concrete steps for the issue A: website Area: website and removed dependencies Pull requests that update a dependency file (automatic) labels May 19, 2020
@mvshmakov
Copy link
Contributor Author

mvshmakov commented May 20, 2020

@shcheklein I guess I have found the issue. I use Visual Studio Code with custom config, which you can see here: https://pastebin.com/qEjpgKsV. It has a section "prettier.useTabs": true which I totally have forgotten of. I suppose you are right about settings overlapping and it was totally my bad.

We also can force this rule this way if it is necessary: prettier/prettier#5019 (comment).

@jorgeorpinel
Copy link
Contributor

jorgeorpinel commented May 20, 2020

I think both VSC and Prettier let tabs in code blocks alone. There's 53 instances of this in content/docs right now.

If there's a way to enforce tab->space with Prettier then I would support the no-tab policy. Otherwise I vote actually to use tabs in sample output. Maybe I'm not getting why that is a problem?

@shcheklein
Copy link
Member

@jorgeorpinel I think it's already enforced in our settings? (useTabs: false) prettier probably just ignores code blocks

@jorgeorpinel
Copy link
Contributor

OK, so my point is if Prettier and IDEs ignore tabs in code blocks (which makes sense) trying to enforce this will be difficult and for starters I don't see why it's desirable. If anything having tabs in sample output is in a way more realistic, as that's what dvc/other commands print to stdout.

@noahbrenner
Copy link

I generally prefer to create an .editorconfig file. Since Prettier also uses this file, that allows .editorconfig to be a central location to configure both your editor/IDE and Prettier.

@jorgeorpinel
Copy link
Contributor

jorgeorpinel commented Aug 19, 2020

Thanks @noahbrenner but I think still Prettier and IDEs will ignore tabls in Markdown code blocks, regardless of how the policy is configured.

@noahbrenner
Copy link

@jorgeorpinel That makes sense. It wouldn't fix existing codeblocks, but I'd expect that an .editorconfig file would at least help avoid creating more instances of this issue in the future (assuming that all contributors have Editor Config plugins for their IDEs, which might not be a good assumption). That doesn't eliminate the need for other automated checks and fixes, it's just another potential tool to help the process along.

@jorgeorpinel
Copy link
Contributor

I'm guessing we are OK to close this since there have been no other comments in a long time, but lmk if you have additional thoughts please. Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A: docs Area: user documentation (gatsby-theme-iterative) A: website Area: website help wanted Contributors especially welcome status: research Writing concrete steps for the issue ⌛ status: wait-response Waiting a response from the outside of the organization type: enhancement Something is not clear, small updates, improvement suggestions
Projects
None yet
Development

No branches or pull requests

4 participants