Make the pretty printer correctly escape field names #1410
Conversation
|
Looks like we are, yes. We probably should stop doing that 😅 |
core/src/pretty.rs
Outdated
| @@ -42,6 +42,13 @@ fn sorted_map<K: Ord, V>(m: &'_ IndexMap<K, V>) -> Vec<(&'_ K, &'_ V)> { | |||
| ret | |||
| } | |||
|
|
|||
| pub fn escape(s: &str) -> String { | |||
There was a problem hiding this comment.
Nitpick, but shouldn't this function be moved to indentifier or even to term::string if it's used inside identifier? it's not obvious that identifier should be depending on pretty
There was a problem hiding this comment.
That's a good point. It felt too unrelated to Ident to go into identifier to me. In fact, Ident::label_quoted is used only in the pretty printer. So maybe the correct course of action to remove this dependency would be to move label_quoted into pretty? I'm not sure.
There was a problem hiding this comment.
Hmm, good point as well. I think both are reasonable:
- Leave
label_quotedin identifier and bring back escape either inidentifierorterm::string, because I believe what you're doing in the end is to escape a Nickel string. It would be like implementingDisplay: I think it's fair to have it inside identifier (or, should we just implementDisplay, by the way, instead oflabel_quoted?) - Move both to
pretty, because it's about pretty-printing stuff
I guess the current in-between with one inside identifier and one inside pretty is what's feeling itchy
There was a problem hiding this comment.
I think I'll go with moving label_quoted into pretty as well, for now.
Previously, the pretty printer relied on `Ident::label_quoted` for printing record field names. This function blindly placed an identifier between quotes if it was deemed necessary. This produces incorrect results if the identifier label itself contains quotes, for example.
vkleen
force-pushed
the
pretty/field-name-quoting
branch
from
8e5f768
to
577a526
Compare
vkleen
force-pushed
the
pretty/field-name-quoting
branch
from
577a526
to
78a10ed
Compare
Previously, the pretty printer relied on
Ident::label_quotedfor printing record field names. This function blindly placed an identifier between quotes if it was deemed necessary. This produces incorrect results if the identifier label itself contains quotes, for example.