-
Notifications
You must be signed in to change notification settings - Fork 26
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey!
If you have any questions about the review, don't hesitate to ask!
@astrale-sharp done |
description: "INPUT\n===\n\"$\\nmat(\\n 1,2,3,4;\\n5,2,2,3\\n)\\n$\"\n===\n$\nmat(\n 1,2,3,4;\n5,2,2,3\n)\n$\n===\nFORMATTED\n===\n$\n mat(1, 2, 3, 4;5, 2, 2, 3)\n$" | ||
expression: formatted | ||
--- | ||
"$\n mat(1, 2, 3, 4;5, 2, 2, 3)\n$" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not sure if this is in scope for this PR but we might want a space to be present after ;
in the formatted result, what's your opinion?
If comma and semicolon are the only things that can appear in math functions as argument separator then maybe, If so it would be good to add a little //TODO in your PR so that we may remember it later!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would like a new line there that makes it similar to the matrix.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't know whether matrix is a special syntax since I cannot find a way to define a function similar to the matrix. If only matrix behaves like this I would even want to align the commas.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should be careful about giving special meaning to ;
and breaking lines, we don't want to end up with
$func_call(a;
b;
c;
)$
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah that makes thing much complicate, and I don't it to delay this pr.
Thanks to these new files I can see the output of your work and it looks really good ! Nice! You're actually one of the rare person to have edited the formatting code itself and not the tooling, I'm curious, was it difficult? Most of the examples you chose are quite alike, I think not all of them are necessary although it's not a big deal to leave them as is. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would add a test like this where the linebreak is not there and the formatter adds it.
"$#xx(a,b) &= 1 \ #xx(a,b) &= 1 \$"
and then we would be good to go, thanks for this nice contribution !
Thanks again for your nice contribution, we're almost good to go! |
Could you do it for me since I will be afk for a while? Thanks 😃 |
I feel like the current implementation won't do it. Maybe I can revise it sometimes. Though maybe some user will prefer to not having a line break unless it is too long? Idk |
Just curious, what would be the output of such a test? (It's okay to add the test to verify that it doesn't break or behave weirdly) |
I didn't see that message, sure! I'll do/test that soon ! |
no worry I can do it now. |
basically without changing anything. |
@astrale-sharp do you think we should add space around |
I'm not sure, it would help to see an example of what you mean! |
With the new test, everything looks good and we're covered so I would be keen to merge as is, but If you want to try with spaces around, we can see what it looks like! |
One test fails, seems like maybe the snapshot was not registered ? |
Ah I made a mistake there. I will fix it tonight EDIT by @astrale-sharp |
Sorry for the delay. Now it should be good. |
No need to apologize! Thanks a lot for this contribution it looks amazing 😁 ! |
); | ||
|
||
if position == 0 && first_align { | ||
should_indent = true; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should_indent = position == 0 && first_align;
Could be rewritten as such, clippy would help
resolve #113
Implement a basic alignment for math block based on the align point. Current rule is:
It scan the syntax twice to retrieve the information (farthest align point). I can't think a way to do it at one pass.
Maybe we can do better for math:
+-/*
) and the align mark&