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

Add option for JSON escaping for placeholder values #527

Closed
wants to merge 1 commit into from

Conversation

akx
Copy link
Contributor

@akx akx commented Mar 21, 2023

Refs #525 (I went ahead and wrote some code...).

This PR

  • adds an option (json_strings) to have ProgressStyle escape all of the dynamic strings it outputs as JSON strings
  • a convenience helper with_json_keys to create a ProgressStyle with a template that outputs a well-formed JSON object

IOW, a ProgressStyle with json_strings(true) and a template

{{"pos": {pos}, "len": {len}, "prefix": {prefix}, "msg": {msg}}}

(this is what ProgressStyle::with_json_keys(&["pos", "len", "prefix", "msg"]) would internally generate) will result in progress lines like

{"pos": "1", "len": "10", "prefix": "hello", "msg": "something"}

which can then be sent to a TermLike target (#354, #526) to implement #525 :)


If you like, I can put this behind a feature gate to avoid the json dependency – but what should indicatif do when trying to do JSON output when it's not compiled in? I'm pretty sure we wouldn't want it to just output non-escaped JSON...

Comment on lines +359 to +391
if self.json_strings {
match style {
Some(s) => cur.push_str(&json::stringify(format!(
"{}",
s.apply_to(padded)
))),
None => cur.push_str(&json::stringify(format!("{padded}"))),
}
} else {
match style {
Some(s) => cur
.write_fmt(format_args!("{}", s.apply_to(padded)))
.unwrap(),
None => cur.write_fmt(format_args!("{padded}")).unwrap(),
}
}
}
None => {
if self.json_strings {
match style {
Some(s) => cur.push_str(&json::stringify(format!(
"{}",
s.apply_to(&buf)
))),
None => cur.push_str(&json::stringify(buf.clone())),
}
} else {
match style {
Some(s) => {
cur.write_fmt(format_args!("{}", s.apply_to(&buf))).unwrap()
}
None => cur.push_str(&buf),
}
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(I'm personally not 100% happy with the complexity here, so if someone has better ideas...)

@djc
Copy link
Collaborator

djc commented May 23, 2023

Sorry for being slow to respond -- while I think your use case is something we'd like to accomodate, I think this PR is probably not the right way to go.

In thinking some more about it, do you think it would be feasible to write a custom ProgressTracker that outputs the JSON? It seems like a viable path to me, in that the impl could take care of all the particular JSON needs.

@akx
Copy link
Contributor Author

akx commented May 23, 2023

@djc No worries! I think I considered that too, but there was something missing in indicatif that was required for that to work. I'll revisit that idea when I have the time :)

@djc
Copy link
Collaborator

djc commented May 23, 2023

Would definitely be open to extending the API available to ProgressTracker if that helps your use case, let me know.

@djc
Copy link
Collaborator

djc commented Aug 1, 2023

Going to close this for now, feel free to reopen if you want to work on this again (or open a new one).

@djc djc closed this Aug 1, 2023
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 this pull request may close these issues.

None yet

2 participants