Fix spaces before \middle #689
Conversation
| * a space. | ||
| */ | ||
| const spliceSpaces = function(children, i) { | ||
| let j = i; |
There was a problem hiding this comment.
Perhaps it would be more intuitive to give i a default value of 0, via let j = i || 0? (as in, e.g., String.prototype.includes)
| i--; | ||
| } else if (spaces) { | ||
| const spaces = spliceSpaces(groups, i); | ||
| if (spaces && i < groups.length) { |
There was a problem hiding this comment.
if (spaces && ...) else if (spaces) ... seems a little weird / hard to understand.
How about if (spaces) { if (i < groups.length) ... else ... }?
There was a problem hiding this comment.
If I thought the code was clearer the way you suggested I would have written it that way. I don't like deeply nested if statements. And it is clearly a matter of taste.
You are asking me for ten minutes or so of work for trivial changes. Like five seconds of typing then ten minutes to rerun the tests just to verify nothing broke, jank around with the GitHub PR, etc. I already spent a lot of time making the fix. As well as writing the original feature—not because I needed it myself, but because I was grateful for KaTeX and wanted to help out with features the main developers wanted.
Projects that want contributions walk a balance between enforcing standards and respecting their contributors' time. As a contributor, I see no standards involved with your comments, and I consider these particular requests less than respectful of my time.
There was a problem hiding this comment.
I'm sorry that you felt your time was disrespected -- not my intent. We certainly appreciate the time you've spent in fixing this bug (and in implementing the original feature). I'll move this agreed-to-be stylistic issue to a separate PR.
There was a problem hiding this comment.
Thanks very much for understanding. I absolutely appreciate your work also, and hope there's no hard feelings. I think it's important that code reviewers for open-source projects understand when they're asking for important functionality changes vs. important stylistic changes vs. tweaks on matters of taste, and thus hope this exchange has been useful for both of us.
| middleDelim.isMiddle.value, innerHeight, innerDepth, | ||
| middleDelim.isMiddle.options, group.mode, []); | ||
| // Add back spaces shifted into the delimiter | ||
| const spaces = spliceSpaces(middleDelim.children, 0); |
There was a problem hiding this comment.
Could use default 0 value of i here.
There was a problem hiding this comment.
Does not seem important.
Shift spaces back into the replacement middle delimiter. Fix #683.