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

Errors include unprintable or awkwardly printed characters. #333

Open
epage opened this issue Jan 10, 2022 · 6 comments · May be fixed by #336
Open

Errors include unprintable or awkwardly printed characters. #333

epage opened this issue Jan 10, 2022 · 6 comments · May be fixed by #336

Comments

@epage
Copy link
Contributor

epage commented Jan 10, 2022

cargo has a test that tries null characters: https://github.com/rust-lang/cargo/pull/10086/files#diff-4980c43ff583070e4cb20d00368e0c15d57a0c0127d7ddc5676e1ee12b899d18R927

it also exposes that a newline is shown for errors: toml-rs/toml#259

@epage
Copy link
Contributor Author

epage commented Jan 10, 2022

It'd be nice if combine did a look up an showed a word or an escape sequence instead of rendering these characters.

I'd be willing to implement this, just want the go ahead first.

@epage
Copy link
Contributor Author

epage commented Jan 10, 2022

I assume we'd either do a debug representation of the string or only do this if the string is 1 character long.

@Marwes
Copy link
Owner

Marwes commented Jan 11, 2022

Just using https://doc.rust-lang.org/std/primitive.str.html#method.escape_default and the same method of char might be good when rendering?

@epage
Copy link
Contributor Author

epage commented Jan 11, 2022

I think the main question is handling of unicode, whether those should be escaped or not. I think most users can see and understand a unicode character but the escaped form is a
bit more mystifying (at least for this ASCII-only person)

It can be a good start though

@epage
Copy link
Contributor Author

epage commented Jan 11, 2022

My proposal is we'd escape

  • unprintable
  • \t, \n, \r
  • Our quote character (back-quote)

I'd personally not escape \ since we are creating something for humans and I think humans would be able to understand the message.

The challenge with any of this is the impl Display for Info is for Info<T: Display, R: Display>. For non byte / char, we could end up munging people's tokens (granted, 99% of custom tokens will probably be unescaped). Though this is making me wonder how we are doing pretty printing of byte tokens which toml_edit is using.

epage added a commit to epage/combine that referenced this issue Jan 11, 2022
@epage epage linked a pull request Jan 11, 2022 that will close this issue
3 tasks
epage added a commit to epage/combine that referenced this issue Jan 11, 2022
epage added a commit to epage/combine that referenced this issue Jan 12, 2022
@Marwes
Copy link
Owner

Marwes commented Jan 13, 2022

That list seems fine to me, it can always be tweaked later.

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 a pull request may close this issue.

2 participants