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

Format to string #1121

Closed
wants to merge 6 commits into from
Closed

Conversation

pitdicker
Copy link
Collaborator

@pitdicker pitdicker commented Jun 3, 2023

This PR is meant as an alternative to #902 and #614, for the problem in described in #575.

The problem is that if the format string is invalid, or contains a field that the type can not provide (such as time fields on a NaiveDate), the display implementation returns an error.

Some formattings macro's can correctly forward this error, such as write! and writeln!. Others, such as println!, format! and to_string panic instead.

Compared to #902 I believe this solution is better because:

  • It works on 0.4.x, no breaking changes
  • It makes it easier to get a String, instead of making it slightly harder to get a DelayedFormat.
  • There is no need to parse the formatting string twice.

There are 5 new methods, all just a couple of lines of code:

  • DelayedFormat::try_to_string
  • NaiveDate::format_to_string
  • NaiveTime::format_to_string
  • NaiveDateTime::format_to_string
  • DateTime::format_to_string

Most of the effort in this PR is updating the surrounding documentation and examples, to make users more aware of the potential panic and the solutions.

@pitdicker pitdicker force-pushed the format_to_string branch 2 times, most recently from c8b2b3f to 8bfcf98 Compare June 3, 2023 08:15
@pitdicker
Copy link
Collaborator Author

Just noticed that we should definitely turn formatting with %#z from a panic into an error.

@pitdicker
Copy link
Collaborator Author

pitdicker commented Jun 4, 2023

Hmm, this is not a complete solution, see #902 (comment).

Still I think format_to_string methods except DelayedFormat::try_to_string have some merit on their own:

  • It works on 0.4.x, no breaking changes
  • It makes it easier to get a String, no need to go through DelayedFormat.
  • There is no need to parse the formatting string twice.

I'll rework this PR a bit.

@pitdicker pitdicker marked this pull request as draft June 4, 2023 11:20
@pitdicker pitdicker marked this pull request as ready for review June 4, 2023 12:17
@pitdicker
Copy link
Collaborator Author

pitdicker commented Jun 4, 2023

@jtmoon79
Copy link
Contributor

jtmoon79 commented Jun 4, 2023

@jtmoon79 You may be interested in this CI failure https://github.com/chronotope/chrono/actions/runs/5169010209/jobs/9310999690?pr=1121

 + curl https://rustwasm.github.io/wasm-pack/installer/init.sh -sSf
info: downloading wasm-pack
curl: (22) The requested URL returned error: 503
wasm-pack-init: failed to download https://github.com/rustwasm/wasm-pack/releases/download/v0.11.1/wasm-pack-v0.11.1-x86_64-unknown-linux-musl.tar.gz
wasm-pack-init: this may be a standard network error, but it may also indicate
wasm-pack-init: that wasm-pack's release process is not working. When in doubt
wasm-pack-init: please feel free to open an issue!
Error: Process completed with exit code 1.

Ah nice, the bash error mode set -euxo pipefail worked as expected.

The curl process failed, very likely a transient error. Instead of the script continuing to run and then hitting some very confusing error later on about missing wasm libraries or unable to load a library or some such thing, the script just halted after curl failed and returned 1. The calling CI process then recognized the error and marked the Job as failed. This is an improvement.

In this case, I suggest re-running the failed CI job.

@djc
Copy link
Member

djc commented Jun 5, 2023

Wrote up my thoughts in #1127.

@pitdicker pitdicker closed this Jun 6, 2023
@pitdicker pitdicker deleted the format_to_string branch July 25, 2023 18:43
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.

3 participants