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

fixing another edge case related to comments + directives #235

Merged
merged 5 commits into from
May 31, 2021

Conversation

belav
Copy link
Owner

@belav belav commented May 27, 2021

refactor docprinter a little
ignore some properties in syntaxNodeComparer so that it gives you a better indication of the code that is failing the validation

@belav belav force-pushed the more-trailing-plus-directive branch from f9c8f22 to 00c85e9 Compare May 27, 2021 02:43
Copy link
Collaborator

@shocklateboy92 shocklateboy92 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, after seeing how this stack gets used, I retract my earlier comment that a List would be better.
In dotnet, a Stack implements IEnumerable, so we can do a read-only iteration through it to evaluate Fits().

Since Js doesn't have a native Stack structure, I can't speak for exactly what prettier used, but I'm guessing they couldn't iterate without mutating their stack, which is why they created a new one.

Src/CSharpier/DocPrinter/DocPrinter.cs Outdated Show resolved Hide resolved
Src/CSharpier/DocPrinter/DocPrinter.cs Outdated Show resolved Hide resolved
@@ -29,7 +29,11 @@ string PrintIndentedDocTree(Doc doc)
case NullDoc:
return indent + "Doc.Null";
case StringDoc stringDoc:
return indent + "\"" + stringDoc.Value?.Replace("\"", "\\\"") + "\"";
if (stringDoc.IsDirective)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: I wonder if this would look better as a:

case StringDoc { IsDirective: true }:
    ...

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is now a giant SwitchExpression which CSharpier doesn't do the greatest job formatting. #237

@belav
Copy link
Owner Author

belav commented May 29, 2021

Why not just iterate over the input list with a foreach (and a .Reverse() if needed), since the only thing we care about is remainingWith?
I was hoping to do that, but when remainingCommands contains Group, Concat etc, then new commands get pushed onto commandToTests.

I'm thinking of doing the following. Fits will be a little more complex, but because of how often it can hit it is probably worth it.

  1. Switching back to a stack in the Print method.
  2. Treating the stack as read only in Fits, and iterating over it like you suggested.
  3. Create another stack in Fits, which is used to push new commands when encountering Group, Concat etc.
  4. Fits will iterate over remainingCommands, and inside of each iteration it will push/pop newCommands until newCommands is empty.

Although now that I type it out, I realize this approach will still be creating a new stack each time Fits gets hit... It could be modified to only create a stack if it needs to. I'll benchmark it and play around.

I did benchmark Stack vs List vs LinkList for this, and List/LinkList were quite a bit faster than Stack, but that was most likely because of the .Reverse used to create the Stack inside of Fits.

belav added 5 commits May 31, 2021 10:19
refactor docprinter a little
ignore some properties in syntaxNodeComparer so that it gives you a better indication of the code that is failing the validation
@belav belav force-pushed the more-trailing-plus-directive branch from d3fbae0 to 5ecf8c8 Compare May 31, 2021 15:20
@belav belav merged commit c7685df into master May 31, 2021
@belav belav deleted the more-trailing-plus-directive branch May 31, 2021 15:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants