[RFC] Indent with tabs #1026

Merged
merged 1 commit into from Apr 7, 2017

Conversation

Projects
None yet
4 participants
@rhengles
Contributor

rhengles commented Mar 16, 2017

I'm really sorry for having to create another PR, don't want to spam this project, but Github doesn't allow me to reopen the previous PR #920 because the branch has been rebased while the PR was closed.

+ indent: ind.indent,
+ align: {
+ spaces: ind.align.spaces + n,
+ tabs: ind.align.tabs + (n ? 1 : 0)

This comment has been minimized.

@rhengles

rhengles Mar 16, 2017

Contributor

@vjeux :

Question to you, if you have nested ternaries and other indentation, you're going to have <tab><space><tab><space>. Isn't it an issue in practice?

When printing code indented with tabs, my current solution replaces all calls to align() with exactly one tab - doesn't matter how many spaces it was aligned with, unless it was zero, then it prints zero tabs also.

@rhengles

rhengles Mar 16, 2017

Contributor

@vjeux :

Question to you, if you have nested ternaries and other indentation, you're going to have <tab><space><tab><space>. Isn't it an issue in practice?

When printing code indented with tabs, my current solution replaces all calls to align() with exactly one tab - doesn't matter how many spaces it was aligned with, unless it was zero, then it prints zero tabs also.

This comment has been minimized.

@vjeux

vjeux Mar 16, 2017

Collaborator

What happens for code like

function f() {
  a
    ? function() {
      a
        ? function() {
            a;
          }
        : c;
      }
    : c;
}
@vjeux

vjeux Mar 16, 2017

Collaborator

What happens for code like

function f() {
  a
    ? function() {
      a
        ? function() {
            a;
          }
        : c;
      }
    : c;
}

This comment has been minimized.

@vjeux

vjeux Mar 16, 2017

Collaborator

I'm especially curious if you have tab width = 3.

The inner one is

<indent><indent><2 space><indent><2 space><indent>

If you do tabWidth = 3 then you have

tab(3) tab(3) space(2) tab(3) space(2) tab(3)

if you output it like this, then the tab after the spaces is going to be width = 1 in order to align, which is incorrect.

What you are doing instead is

tab(3) tab(3) tab(3) space(2) space(2)

which seems to be correct!

@vjeux

vjeux Mar 16, 2017

Collaborator

I'm especially curious if you have tab width = 3.

The inner one is

<indent><indent><2 space><indent><2 space><indent>

If you do tabWidth = 3 then you have

tab(3) tab(3) space(2) tab(3) space(2) tab(3)

if you output it like this, then the tab after the spaces is going to be width = 1 in order to align, which is incorrect.

What you are doing instead is

tab(3) tab(3) tab(3) space(2) space(2)

which seems to be correct!

@rhengles rhengles referenced this pull request Mar 16, 2017

Closed

[RFC] Indent with tabs #920

@vjeux

This comment has been minimized.

Show comment
Hide comment
@vjeux

vjeux Mar 16, 2017

Collaborator

@jlongster I actually think that the solution we sketched out isn't going to work well for nested spaces that are "fixed". It seems also like a good thing to have two different apis for fixed and variable indentation.

Could you review this pull request again? Thanks!

Collaborator

vjeux commented Mar 16, 2017

@jlongster I actually think that the solution we sketched out isn't going to work well for nested spaces that are "fixed". It seems also like a good thing to have two different apis for fixed and variable indentation.

Could you review this pull request again? Thanks!

@jlongster

This comment has been minimized.

Show comment
Hide comment
@jlongster

jlongster Mar 21, 2017

Member

I will review this soon, sorry. It's a bigger thing to talk about so after I go through all the small issues/PRs I end up not having time to tackle this :) I will try to look at this later tonight or tomorrow morning.

It sounds like we're converging on a solution and this is looking good. I'm not stuck on the approach I was suggesting before.

Member

jlongster commented Mar 21, 2017

I will review this soon, sorry. It's a bigger thing to talk about so after I go through all the small issues/PRs I end up not having time to tackle this :) I will try to look at this later tonight or tomorrow morning.

It sounds like we're converging on a solution and this is looking good. I'm not stuck on the approach I was suggesting before.

@jlongster

This comment has been minimized.

Show comment
Hide comment
@jlongster

jlongster Apr 5, 2017

Member

I am so sorry I neglected this. A few things came up the last few weeks and I had to focus on client or other work. I'll be looking through prettier again over the next few days, and I'll look at this issue later tonight and review it. (It's probably ready, but I'll take one pass at it)

Member

jlongster commented Apr 5, 2017

I am so sorry I neglected this. A few things came up the last few weeks and I had to focus on client or other work. I'll be looking through prettier again over the next few days, and I'll look at this issue later tonight and review it. (It's probably ready, but I'll take one pass at it)

@rhengles

This comment has been minimized.

Show comment
Hide comment
@rhengles

rhengles Apr 6, 2017

Contributor

I will push now a rebase on top of the recent PRs. 👍

Contributor

rhengles commented Apr 6, 2017

I will push now a rebase on top of the recent PRs. 👍

src/doc-printer.js
@@ -35,7 +62,11 @@ function fits(next, restCommands, width) {
break;
case "indent":
- cmds.push([ind + doc.n, mode, doc.contents]);
+ cmds.push([indent(ind), mode, doc.contents]);

This comment has been minimized.

@jlongster

jlongster Apr 6, 2017

Member

It's a little confusing to name these functions indent and align, the same as the "commands". Can you possibly rename these? At first I thought it was calling the command and was very confused. Maybe something like makeIndent?

@jlongster

jlongster Apr 6, 2017

Member

It's a little confusing to name these functions indent and align, the same as the "commands". Can you possibly rename these? At first I thought it was calling the command and was very confused. Maybe something like makeIndent?

@jlongster

This comment has been minimized.

Show comment
Hide comment
@jlongster

jlongster Apr 6, 2017

Member

Alright, thanks for describing the reasons for this algorithm and being patient. This looks good to me. I made one small comment where I think there could be some clarification, but otherwise this is fine.

Member

jlongster commented Apr 6, 2017

Alright, thanks for describing the reasons for this algorithm and being patient. This looks good to me. I made one small comment where I think there could be some clarification, but otherwise this is fine.

Refactored option to indent with tabs
Refactored option to indent with tabs
@rhengles

This comment has been minimized.

Show comment
Hide comment
@rhengles

rhengles Apr 6, 2017

Contributor

@jlongster Fixed! Thank you very much! 😃 Please forgive me for my exaggerated enthusiasm sometimes... 😄

Contributor

rhengles commented Apr 6, 2017

@jlongster Fixed! Thank you very much! 😃 Please forgive me for my exaggerated enthusiasm sometimes... 😄

@jlongster

This comment has been minimized.

Show comment
Hide comment
@jlongster

jlongster Apr 7, 2017

Member

That was quick! No worries at all, I took it as just being thorough. Let's merge this and see if any problems come up!

Member

jlongster commented Apr 7, 2017

That was quick! No worries at all, I took it as just being thorough. Let's merge this and see if any problems come up!

@jlongster jlongster merged commit 170e4d5 into prettier:master Apr 7, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

This was referenced Apr 7, 2017

@respectTheCode

This comment has been minimized.

Show comment
Hide comment
@respectTheCode

respectTheCode Apr 7, 2017

are there plans to do a release with this?

are there plans to do a release with this?

@jlongster

This comment has been minimized.

Show comment
Hide comment
@jlongster

jlongster Apr 7, 2017

Member

@respectTheCode Definitely, not sure exactly when though. In the next few days at the latest.

Member

jlongster commented Apr 7, 2017

@respectTheCode Definitely, not sure exactly when though. In the next few days at the latest.

@gziolo gziolo referenced this pull request in Automattic/wp-calypso Apr 14, 2017

Closed

Framework: Use Prettier? #12260

@zhenyulin zhenyulin referenced this pull request in prettier/eslint-plugin-prettier Jan 29, 2018

Closed

[RFF]support tab as indent #82

@rhengles rhengles referenced this pull request in belezanaweb/test-front Jun 18, 2018

Closed

Estrutura de desenvolvimento #1

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment