Support symbols #42
Support symbols #42
Conversation
| Ok(#(_, "test")) -> Ok(filepath.join(cases_directory, file)) | ||
| _ -> Error(Nil) | ||
| } | ||
| }) |
There was a problem hiding this comment.
Could we write this as a call to filter with string.ends_with? I think that'd be clearer. Took me a moment to understand what this is doing
There was a problem hiding this comment.
This has not been done! It's still using filter_map
There was a problem hiding this comment.
Oh I missed your point about filter vs. filter_map. The filtered list will still need to be mapped (or vice versa) so I figured on saving an iteration.
There was a problem hiding this comment.
It only runs once per test run with a tiny amount of data so clarity is preferred here 🙏
e.g. vim swap files, which was making it annoying to tweak a test case because I kept having to close the file for the tests to run.
|
Repushed, thanks. |
lpil
left a comment
lpil
left a comment
There was a problem hiding this comment.
Thank you, couple more notes inline.
|
|
||
| fn parse_symbol(in: String) -> Option(#(Inline, String)) { | ||
| take_symbol_chars(in, "") | ||
| |> option.map(pair.map_first(_, Symbol)) |
There was a problem hiding this comment.
Use a case expression instead of combinators please, it is both faster and easier to understand.
| // the regexp [-_+a-zA-Z09]+, see: | ||
| // https://github.com/jgm/djoths/blob/83dbadd6aa325ff23f0e4144221b0df2c64becc7/src/Djot/Inlines.hs#L213-L221 | ||
| fn take_symbol_chars(in: String, acc: String) -> Option(#(String, String)) { | ||
| case string.pop_grapheme(in) { |
There was a problem hiding this comment.
Please use the iteration technique used in the existing code where the string is pattern matched on 🙏 Pattern matching is very fast, and performance is a priority in this package.
There was a problem hiding this comment.
I found pop_grapheme in use in comparable iterations in the library so I copied that pattern. I can change it but there is apparently not a unique iteration pattern used in the library so your first review comment to this effect led me to a solution you don’t prefer.
| Ok(#(_, "test")) -> Ok(filepath.join(cases_directory, file)) | ||
| _ -> Error(Nil) | ||
| } | ||
| }) |
There was a problem hiding this comment.
This has not been done! It's still using filter_map
2 participants
the html rendering is identical to what djoths and thus pandoc do: you get a
<span class="symbol">contents</span>. There's an argument for making this a knob and just rendering the symbols as plain text if not requested but I'm not here to add knobs.PR contains an unrelated tweak to the test helpers which you are free to drop.