[RFC] Indent with tabs#1026
Conversation
| indent: ind.indent, | ||
| align: { | ||
| spaces: ind.align.spaces + n, | ||
| tabs: ind.align.tabs + (n ? 1 : 0) |
There was a problem hiding this comment.
@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.
There was a problem hiding this comment.
What happens for code like
function f() {
a
? function() {
a
? function() {
a;
}
: c;
}
: c;
}There was a problem hiding this comment.
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!
|
@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! |
rhengles
force-pushed
the
indent-tabs-refactor
branch
7 times, most recently
from
d82126e to
01e10cb
Compare
|
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. |
rhengles
force-pushed
the
indent-tabs-refactor
branch
3 times, most recently
from
f255266 to
49c30eb
Compare
|
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) |
|
I will push now a rebase on top of the recent PRs. 👍 |
rhengles
force-pushed
the
indent-tabs-refactor
branch
from
49c30eb to
f3d3baa
Compare
| break; | ||
| case "indent": | ||
| cmds.push([ind + doc.n, mode, doc.contents]); | ||
| cmds.push([indent(ind), mode, doc.contents]); |
There was a problem hiding this comment.
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?
|
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
rhengles
force-pushed
the
indent-tabs-refactor
branch
from
f3d3baa to
ed3a482
Compare
|
@jlongster Fixed! Thank you very much! 😃 Please forgive me for my exaggerated enthusiasm sometimes... 😄 |
|
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! |
|
are there plans to do a release with this? |
|
@respectTheCode Definitely, not sure exactly when though. In the next few days at the latest. |
lock
Bot
added
the
locked-due-to-inactivity
4 participants
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.