-
-
Notifications
You must be signed in to change notification settings - Fork 55
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
Support diacritics (accents, ...) #223
Conversation
Codecov Report
@@ Coverage Diff @@
## master #223 +/- ##
==========================================
- Coverage 63.84% 63.18% -0.67%
==========================================
Files 23 23
Lines 838 861 +23
==========================================
+ Hits 535 544 +9
- Misses 303 317 +14
Continue to review full report at Codecov.
|
02ea5d0
to
2cc64c0
Compare
4dcd428
to
2d87c03
Compare
5655e94
to
f52e951
Compare
Hi! Thank you for the contribution, this looks like a good idea, but I haven't done a proper review yet. However, I feel the need to comment on your workflow. You seem to be force-pushing a lot. That's a pretty extreme measure, and makes it difficult to follow your history. Please make successive commits rather than amending a single commit and force-pushing it. I'm certain most repos will agree with this. |
Thanks, I think this can be reviewed in a few moments.
Pushing a lot of commits is certainly a bad idea too. I can rebase this PR though, to make things clearer. |
I would certainly prefer several commits over a constantly changing history. If your want to cause fewer commits, consider simply pushing less often. |
Rebased for clarity. The last commit is optional and can be removed if required. |
f50f3b5
to
de9e2fc
Compare
@gustaphe, would it be possible to review (and merge this PR / bump ver iff approved) so that it does not block JuliaPlots/Plots.jl#4262 ? |
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 disagree with the choice of making all of these \textrm
. If a character in math mode is supposed to be romanized, then that should be a property of the variable it signifies, not its spelling. To me it's better to get an Command \" invalid in math mode
error than have it silently put some characters in roman.
Co-authored-by: gustaphe <david.e.gustavsson@gmail.com>
Thanks for the detailed review: I've tried to answer all the comments.
I'm not sure I'm following you here: the point of |
This is a very well-constructed PR with good code quality and with tests. Thank you! As to the above discussion about |
That may be right. My objection is exemplified in this hypothetical: say I define a type struct TextSub
var::String
sub::String
end
@latexrecipe function f(x::TextSub)
return Expr(:latexifymerge,
x.var,
"_\textrm{",
x.sub,
"}"
)
end (Possible misspellings, I'm on my phone traveling) This is how I would use anything where I expected romanized text, and so anything where I would expect There is precedent for using kwargs to introduce formatting commands on demand, could that be the solution here? That said, maybe this is an inconsequential objection. I'll survive. |
I fail to understand how your example shows a valid point since it doesn't compile: using Latexify
struct TextSub
var::String
sub::String
end
@latexrecipe f(x::TextSub) = return Expr(:latexifymerge, x.var, raw"_\textrm{", x.sub, "}")
main() = begin
fn = tempname()
write(
fn, """
\\documentclass{standalone}
\\begin{document}
$(latexify(:($(TextSub("é", "ç")) + $(TextSub("c", "123")))))
\\end{document}
"""
)
run(`latexmk -quiet -pdf $fn`)
return
end
main() with PR
without PR
I understand your concerns about |
Thanks ! Don't hesitate to ping me if any issue pops up related to this PR. |
QuoteNode
inlatexraw
(e.g. when parsing"Plots.jl"
as expr, corner case);latexify
docstring forhelp?> latexify
;parse
keyword to_latexraw
, sinceMeta.parse
can fail (especially parsing string containing diacritric markers - strings are normalized), and dispatch onVal{true/false}
toMeta.parse
or not.Fix #182.
Goes with JuliaPlots/Plots.jl#4262.