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

Prettier v1.11.1 is mixing spaces and tabs for alignment #4199

Closed
kiruh opened this issue Mar 24, 2018 · 23 comments
Closed

Prettier v1.11.1 is mixing spaces and tabs for alignment #4199

kiruh opened this issue Mar 24, 2018 · 23 comments
Labels
lang:javascript Issues affecting JS locked-due-to-inactivity Please open a new issue and fill out the template instead of commenting. type:question Questions and support requests. Please use Stack Overflow for them, not the issue tracker.

Comments

@kiruh
Copy link

kiruh commented Mar 24, 2018

Prettier 1.11.1
Playground link

# Options
--use-tabs
--tab-width 4

Input:

const store = useLoggerAndDevTools
	? createStore(
			reducer,
			logRedux
				? compose(applyMiddleware(createLogger(), thunk))
				: compose(applyMiddleware(thunk))
>		)
	: createStore(reducer, applyMiddleware(thunk));

Output:

const store = useLoggerAndDevTools
	? createStore(
			reducer,
			logRedux
				? compose(applyMiddleware(createLogger(), thunk))
				: compose(applyMiddleware(thunk))
>	  )
	: createStore(reducer, applyMiddleware(thunk));

Expected behaviour (as it were in v1.10.2):

const store = useLoggerAndDevTools
	? createStore(
			reducer,
			logRedux
				? compose(applyMiddleware(createLogger(), thunk))
				: compose(applyMiddleware(thunk))
>		)
	: createStore(reducer, applyMiddleware(thunk));

v1.11.1 is mixing tabs and spaces as shown in output. ESLint is throwing an exception no-mixed-spaces-and-tabs.

Is it a bug? If not maybe there should be an option for mixing spaces and tabs for alignment.

@lydell
Copy link
Member

lydell commented Mar 24, 2018

As far as I know, it is on purpose. I think essentially the same discussion is currently going on in #4198.

Since you mention ESLint … if that's your only issue the solution is easy. We recommend turning off all conflicting ESLint rules: https://prettier.io/docs/en/eslint.html#turn-off-eslint-s-formatting-rules

@kiruh
Copy link
Author

kiruh commented Mar 24, 2018

@lydell I know that it's possible to disable ESLint formatting rules, but mixed spaces and tabs just don't look very good (for me).

I've heard:

Use tabs for indentation, spaces for alignment.

But maybe it should be optional. Because for me 2 tabs look more readable than 1 tab and 2 spaces. Even if it's not aligned.

In #4198 the main discussion is about nested tabs.

@lydell
Copy link
Member

lydell commented Mar 25, 2018

I know that it's possible to disable ESLint formatting rules, but mixed spaces and tabs just don't look very good (for me).

Pro-tip: Actually say so from the beginning then! Thanks :)

@thorn0
Copy link
Member

thorn0 commented Mar 26, 2018

@kiruh How does this indentation look to you (let's pretend that those spaces are tabs)?

const store = useLoggerAndDevTools
    ? createStore(
        reducer,
        logRedux
            ? compose(applyMiddleware(createLogger(), thunk))
            : compose(applyMiddleware(thunk))
    )
    : createStore(reducer, applyMiddleware(thunk));

@kiruh
Copy link
Author

kiruh commented Mar 27, 2018

How does this indentation look to you (let's pretend that those spaces are tabs)?

@thorn0 It looks good.

@alexgv99
Copy link

alexgv99 commented Apr 4, 2018

You have to change the eslint rule so it support smart-tabs:
"no-mixed-spaces-and-tabs": ["warn", "smart-tabs"],

@lydell
Copy link
Member

lydell commented Apr 4, 2018

Even better: "no-mixed-spaces-and-tabs": "off". Or use eslint-config-prettier :)

@suchipi suchipi added type:question Questions and support requests. Please use Stack Overflow for them, not the issue tracker. status:awaiting response Issues that require answers to questions from maintainers before action can be taken labels Apr 5, 2018
@kiruh
Copy link
Author

kiruh commented Apr 5, 2018

I know that it's possible to disable ESLint formatting rules, but mixed spaces and tabs just don't look very good (for me).

But maybe it should be optional. Because for me 2 tabs look more readable than 1 tab and 2 spaces. Even if it's not aligned.

@no-response no-response bot removed the status:awaiting response Issues that require answers to questions from maintainers before action can be taken label Apr 5, 2018
@lydell lydell added status:needs discussion Issues needing discussion and a decision to be made before action can be taken lang:javascript Issues affecting JS and removed type:question Questions and support requests. Please use Stack Overflow for them, not the issue tracker. labels Apr 5, 2018
@suchipi
Copy link
Member

suchipi commented Apr 5, 2018

Sorry; thanks for the clarification.

I don't have a horse in this race since I don't use tabs. I'll let those with informed opinions discuss this.

@teameh
Copy link

teameh commented Apr 25, 2018

I don't care either about the default. But I expect "no-mixed-spaces-and-tabs": ["error"], to result in no smart-tabs when using eslint-config-prettier. Currently that is not the case, I keep getting spaces.

@lydell
Copy link
Member

lydell commented Apr 25, 2018

@teameh Please open an issue in eslint-config-prettier if there’s something it could do better :)

@j-f1
Copy link
Member

j-f1 commented Apr 25, 2018

@teameh Prettier’s use of mixed spaces and tabs is not configurable. eslint-config-prettier is designed to disable conflicting rules, not to alter Prettier’s behavior. We recommend removing all formatting-related rules from your ESLint config.

@kiruh
Copy link
Author

kiruh commented Apr 25, 2018

Prettier’s use of mixed spaces and tabs is not configurable.

@j-f1 Should it become configurable in future?

@j-f1
Copy link
Member

j-f1 commented Apr 25, 2018

@kiruh Not unless there’s lots of support — see our option philosophy.

@thorn0
Copy link
Member

thorn0 commented Apr 26, 2018

I wonder if there exists a tab user who would oppose to a spaceless formatting like this:

  1. Prettier v1.11.1 is mixing spaces and tabs for alignment #4199 (comment)
  2. TypeScript: nested union types + useTabs #4198 (comment)

If there is no any, what is needed here is not configurability then.

@jankalfus
Copy link

This is most definitely an issue. Why would you want to ever mix spaces and tabs? It just doesn't feel right.

@duailibe
Copy link
Member

duailibe commented May 16, 2018

@jankalfus Sorry that's an issue. Unfortunately saying it "doesn't feel right" adds nothing to the discussion. And the "why" has already been discussed above.

If you want to help, we'll take suggestions on how to improve this (no options), with real code examples. Or PRs.

@thorn0 Are you interested in opening a PR with your suggestion?

@jankalfus
Copy link

@duailibe Sorry for not being specific. I guess my question would be: Why has this even been introduced? What is the reasoning behind it?

I simply don't understand why someone would want to mix tabs and spaces. Generally, if spaces are used, then it's because it is required that the code looks the same on all people's monitors. If tabs are used, then everybody can use whatever tab size they prefer. If I change tab size on my machine, the code will look broken with mixed spaces.

@duailibe
Copy link
Member

duailibe commented May 16, 2018

@jankalfus Because prettier aligns everything after ? (in the OP that would be createStore aligned with the closing )).

cond
  ? {
      foo
    } // this is aligned with the opening bracket
  : foo;

I simply don't understand why someone would want to mix tabs and spaces

Again this is the kind of statement that adds nothing to discussion and doesn't help getting your problem fixed. This is not a support center. We'll take suggestions but we don't need complaints.

@jankalfus
Copy link

@duailibe I gave you an explanation. Taking a single sentence out of context doesn't make the rest of my post irrelevant.

@ikatyang
Copy link
Member

If tabs are used, then everybody can use whatever tab size they prefer.

The question is, tabs-only is not aligned, which is why someone raised #2771.

const store = useLoggerAndDevTools
	? createStore(
			reducer,
			logRedux
				? compose(applyMiddleware(createLogger(), thunk))
				: compose(applyMiddleware(thunk))
		) // <------------
	: createStore(reducer, applyMiddleware(thunk));

If I change tab size on my machine, the code will look broken with mixed spaces.

What do you mean broken? There shouldn't be any visual difference with or without --use-tabs.

@jankalfus
Copy link

Scratch everything I said. You're right, it doesn't get broken with mixed tabs and spaces, I just realized it. Sorry!

@suchipi
Copy link
Member

suchipi commented May 24, 2018

I'm going to close this as intended behavior. The arguments against doing this seem to be based on personal preference. However, if someone has a bad experience in their editor because of this and can explain why, I would be open to reconsidering.

@suchipi suchipi closed this as completed May 24, 2018
@suchipi suchipi added type:question Questions and support requests. Please use Stack Overflow for them, not the issue tracker. and removed status:needs discussion Issues needing discussion and a decision to be made before action can be taken labels May 24, 2018
@lock lock bot added the locked-due-to-inactivity Please open a new issue and fill out the template instead of commenting. label Aug 22, 2018
@lock lock bot locked as resolved and limited conversation to collaborators Aug 22, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
lang:javascript Issues affecting JS locked-due-to-inactivity Please open a new issue and fill out the template instead of commenting. type:question Questions and support requests. Please use Stack Overflow for them, not the issue tracker.
Projects
None yet
Development

No branches or pull requests

10 participants