feat: reinterpret `precision` field for strings #34544

3Hren · 2016-06-29T07:52:56Z

This commit changes the behavior of formatting string arguments with both width and precision fields set.

Documentation says that the width field is the "minimum width" that the format should take up. If the value's string does not fill up this many characters, then the padding specified by fill/alignment will be used to take up the required space.

This is true for all formatted types except string, which is truncated down to precision number of chars and then all of fill, align and width fields are completely ignored.

For example: format!("{:/^10.8}", "1234567890); emits "12345678". In the contrast Python version works as the expected:

>>> '{:/^10.8}'.format('1234567890')
'/12345678/'

This commit gives back the Python behavior by changing the precision field meaning to the truncation and nothing more. The result string will be prepended/appended up to the width field with the proper fill char.

However, this is the breaking change, I admit. Feel free to close it, but otherwise it should be mentioned in the std::fmt documentation somewhere near of fill/align/width fields description.

rust-highfive · 2016-06-29T07:53:05Z

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @aturon (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

alexcrichton · 2016-07-05T18:15:43Z

Thanks for the PR @3Hren! While this does change behavior and could be a breaking change, to me this seems like the correct interpretation of the documentation as well so I'd consider it a bug fix. I agree that this is the more correct way to do this, and I like the size of the patch as well! Would it be possible to clarify in the documentation a bit when this case arises this is what happens?

cc @rust-lang/libs, others have thoughts on a change like this?

sfackler · 2016-07-05T18:22:08Z

This seems reasonable to me. I'm not too worried about it breaking things.

brson · 2016-07-05T18:47:19Z

sgtm

3Hren · 2016-07-05T19:52:46Z

@alexcrichton thanks for reviewing this PR! Just updated the docs, please take a look.

Should I squash commits?

alexcrichton · 2016-07-05T20:55:01Z

Looks good to me, thanks! And yeah I'd recommend squashing.

This commit changes the behavior of formatting string arguments with both width and precision fields set. Documentation says that the `width` field is the "minimum width" that the format should take up. If the value's string does not fill up this many characters, then the padding specified by fill/alignment will be used to take up the required space. This is true for all formatted types except string, which is truncated down to `precision` number of chars and then all of `fill`, `align` and `width` fields are completely ignored. For example: `format!("{:/^10.8}", "1234567890);` emits "12345678". In the contrast Python version works as the expected: ```python >>> '{:/^10.8}'.format('1234567890') '/12345678/' ``` This commit gives back the `Python` behavior by changing the `precision` field meaning to the truncation and nothing more. The result string *will* be prepended/appended up to the `width` field with the proper `fill` char. However, this is the breaking change. Also updated `std::fmt` docs about string precision. Signed-off-by: Evgeny Safronov <division494@gmail.com>

3Hren · 2016-07-05T21:02:12Z

Done!

3Hren · 2016-07-08T10:34:15Z

@alexcrichton should I do anything else here?

I see that this PR suddenly became unmergable for some reasons.

alexcrichton · 2016-07-08T15:55:40Z

@3Hren nah you're good, it'll take a moment for @rust-lang/libs to convene about this most likely, but it's all on our end now at this point! I'll be sure to let you know if we need anything else.

This looks to me like it's still mergeable, but if that happens it may just indicate that this needs to be rebased and force-pushed to your branch.

alexcrichton · 2016-07-19T04:20:55Z

Discussed today the libs team decided to merge, thanks again @3Hren!

@bors: r+

bors · 2016-07-19T04:20:56Z

📌 Commit ede39ae has been approved by alexcrichton

bors · 2016-07-19T06:54:08Z

⌛ Testing commit ede39ae with merge ac8b776...

bors · 2016-07-19T08:17:48Z

💔 Test failed - auto-win-gnu-32-opt-rustbuild

alexcrichton · 2016-07-19T15:39:58Z

@bors: retry

maybe Spurious timeout starting to run tests on Windows #33361

bors · 2016-07-19T16:08:20Z

⌛ Testing commit ede39ae with merge 0c737f1...

bors · 2016-07-19T16:54:43Z

💔 Test failed - auto-win-msvc-64-cargotest

alexcrichton · 2016-07-19T16:58:35Z

@bors: retry

That bot isn't having a good day

On Tue, Jul 19, 2016 at 9:54 AM, bors notifications@github.com wrote:

💔 Test failed - auto-win-msvc-64-cargotest
https://buildbot.rust-lang.org/builders/auto-win-msvc-64-cargotest/builds/1150

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#34544 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/AAD95Ir62uAKJX6bEqm7b6Dm-ik0KGoMks5qXQFWgaJpZM4JA3OD
.

bors · 2016-07-19T19:36:24Z

⌛ Testing commit ede39ae with merge aa501e2...

bors · 2016-07-19T19:38:19Z

💔 Test failed - auto-win-msvc-64-opt

alexcrichton · 2016-07-19T19:54:54Z

@bors: retry

Cascading failure on bots when interrupted when performing git operations #34595

On Tue, Jul 19, 2016 at 12:38 PM, bors notifications@github.com wrote:

💔 Test failed - auto-win-msvc-64-opt
https://buildbot.rust-lang.org/builders/auto-win-msvc-64-opt/builds/4934

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#34544 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/AAD95BAtOFxS1QrtQGubn8MtxGD68dGDks5qXSeugaJpZM4JA3OD
.

3Hren · 2016-07-19T20:11:25Z

He hates me

bors · 2016-07-20T04:06:17Z

⌛ Testing commit ede39ae with merge 4e8750d...

bors · 2016-07-20T04:10:11Z

💔 Test failed - auto-win-gnu-32-opt-rustbuild

alexcrichton · 2016-07-20T05:15:30Z

@bors: retry

Cascading failure on bots when interrupted when performing git operations #34595

On Tue, Jul 19, 2016 at 9:10 PM, bors notifications@github.com wrote:

💔 Test failed - auto-win-gnu-32-opt-rustbuild
https://buildbot.rust-lang.org/builders/auto-win-gnu-32-opt-rustbuild/builds/1822

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#34544 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/AAD95DAru5g1rB30jrKLkHF5jT47ftvRks5qXZ-ngaJpZM4JA3OD
.

bors · 2016-07-20T12:46:30Z

⌛ Testing commit ede39ae with merge 3329c1d...

bors · 2016-07-20T13:23:52Z

💔 Test failed - auto-linux-64-cross-freebsd

alexcrichton · 2016-07-20T17:16:36Z

@bors: retry

network weirdness

On Wed, Jul 20, 2016 at 6:23 AM, bors notifications@github.com wrote:

💔 Test failed - auto-linux-64-cross-freebsd
https://buildbot.rust-lang.org/builders/auto-linux-64-cross-freebsd/builds/995

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#34544 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/AAD95Jq-zT4tucnv5-TQORr8NrDMNiCwks5qXiFqgaJpZM4JA3OD
.

bors · 2016-07-20T23:49:38Z

⌛ Testing commit ede39ae with merge 372472b...

bors · 2016-07-21T00:31:07Z

💔 Test failed - auto-win-gnu-32-opt-rustbuild

alexcrichton · 2016-07-21T07:13:04Z

@bors: retry

On Wed, Jul 20, 2016 at 5:31 PM, bors notifications@github.com wrote:

💔 Test failed - auto-win-gnu-32-opt-rustbuild
https://buildbot.rust-lang.org/builders/auto-win-gnu-32-opt-rustbuild/builds/1859

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#34544 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/AAD95ITzQTKbgIAALzYf3pY0fA28RnUtks5qXr3OgaJpZM4JA3OD
.

bors · 2016-07-21T08:35:51Z

⌛ Testing commit ede39ae with merge de06ee9...

bors · 2016-07-21T11:05:33Z

💔 Test failed - auto-win-gnu-64-opt

alexcrichton · 2016-07-21T15:56:28Z

Never seen that error before... Assuming spurious though as it seems unrelated!

@bors: retry

bors · 2016-07-21T17:22:09Z

⌛ Testing commit ede39ae with merge 4a1775d...

…r-strings, r=alexcrichton feat: reinterpret `precision` field for strings This commit changes the behavior of formatting string arguments with both width and precision fields set. Documentation says that the `width` field is the "minimum width" that the format should take up. If the value's string does not fill up this many characters, then the padding specified by fill/alignment will be used to take up the required space. This is true for all formatted types except string, which is truncated down to `precision` number of chars and then all of `fill`, `align` and `width` fields are completely ignored. For example: `format!("{:/^10.8}", "1234567890);` emits "12345678". In the contrast Python version works as the expected: ```python >>> '{:/^10.8}'.format('1234567890') '/12345678/' ``` This commit gives back the `Python` behavior by changing the `precision` field meaning to the truncation and nothing more. The result string *will* be prepended/appended up to the `width` field with the proper `fill` char. __However, this is the breaking change, I admit.__ Feel free to close it, but otherwise it should be mentioned in the `std::fmt` documentation somewhere near of `fill/align/width` fields description.

bors · 2016-07-21T17:57:19Z

💔 Test failed - auto-linux-musl-64-opt

alexcrichton · 2016-07-21T18:04:35Z

@bors: retry

network error... again?

On Thu, Jul 21, 2016 at 10:57 AM, bors notifications@github.com wrote:

💔 Test failed - auto-linux-musl-64-opt
https://buildbot.rust-lang.org/builders/auto-linux-musl-64-opt/builds/5027

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#34544 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/AAD95F6d909S2ZfipCnlcOAQ43_pMOc3ks5qX7MCgaJpZM4JA3OD
.

bors · 2016-07-21T18:47:33Z

⌛ Testing commit ede39ae with merge ea06463...

bors · 2016-07-21T18:51:04Z

💔 Test failed - auto-win-gnu-32-opt

alexcrichton · 2016-07-21T20:28:42Z

@bors: retry

sorry for the number of retries...

On Thu, Jul 21, 2016 at 11:51 AM, bors notifications@github.com wrote:

💔 Test failed - auto-win-gnu-32-opt
https://buildbot.rust-lang.org/builders/auto-win-gnu-32-opt/builds/5015

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#34544 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/AAD95KZELzjiMU4TPkl_X1plXGQGCyfaks5qX7-bgaJpZM4JA3OD
.

bors · 2016-07-21T23:19:54Z

⌛ Testing commit ede39ae with merge 62690b3...

…r-strings, r=alexcrichton feat: reinterpret `precision` field for strings This commit changes the behavior of formatting string arguments with both width and precision fields set. Documentation says that the `width` field is the "minimum width" that the format should take up. If the value's string does not fill up this many characters, then the padding specified by fill/alignment will be used to take up the required space. This is true for all formatted types except string, which is truncated down to `precision` number of chars and then all of `fill`, `align` and `width` fields are completely ignored. For example: `format!("{:/^10.8}", "1234567890);` emits "12345678". In the contrast Python version works as the expected: ```python >>> '{:/^10.8}'.format('1234567890') '/12345678/' ``` This commit gives back the `Python` behavior by changing the `precision` field meaning to the truncation and nothing more. The result string *will* be prepended/appended up to the `width` field with the proper `fill` char. __However, this is the breaking change, I admit.__ Feel free to close it, but otherwise it should be mentioned in the `std::fmt` documentation somewhere near of `fill/align/width` fields description.

3Hren · 2016-07-21T23:36:32Z

It’s all right. Just robots dominating :D

tamird · 2016-07-22T01:03:35Z

src/libcore/fmt/mod.rs

-            // case where the maximum length will matter.
+        // string being formatted.
+        let s = if let Some(max) = self.precision {
+            // If our string is longer that the precision, then we must have


s/that/than/

bors · 2016-07-22T02:44:57Z

rust-highfive assigned aturon Jun 29, 2016

alexcrichton added the T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. label Jun 29, 2016

brson added the relnotes Marks issues that should be documented in the release notes of the next release. label Jul 5, 2016

3Hren force-pushed the issue/xx/reinterpret-format-precision-for-strings branch from bcd0e44 to ede39ae Compare July 5, 2016 21:01

tamird reviewed Jul 22, 2016
View reviewed changes

bors merged commit ede39ae into rust-lang:master Jul 22, 2016

feat: reinterpret precision field for strings #34544

feat: reinterpret precision field for strings #34544

Conversation

3Hren commented Jun 29, 2016 • edited Loading

rust-highfive commented Jun 29, 2016

alexcrichton commented Jul 5, 2016

sfackler commented Jul 5, 2016

brson commented Jul 5, 2016

3Hren commented Jul 5, 2016

alexcrichton commented Jul 5, 2016

3Hren commented Jul 5, 2016

3Hren commented Jul 8, 2016

alexcrichton commented Jul 8, 2016

alexcrichton commented Jul 19, 2016

bors commented Jul 19, 2016

bors commented Jul 19, 2016

bors commented Jul 19, 2016

alexcrichton commented Jul 19, 2016

bors commented Jul 19, 2016

bors commented Jul 19, 2016

alexcrichton commented Jul 19, 2016

bors commented Jul 19, 2016

bors commented Jul 19, 2016

alexcrichton commented Jul 19, 2016

3Hren commented Jul 19, 2016

bors commented Jul 20, 2016

bors commented Jul 20, 2016

alexcrichton commented Jul 20, 2016

bors commented Jul 20, 2016

bors commented Jul 20, 2016

alexcrichton commented Jul 20, 2016

bors commented Jul 20, 2016

bors commented Jul 21, 2016

alexcrichton commented Jul 21, 2016

bors commented Jul 21, 2016

bors commented Jul 21, 2016

alexcrichton commented Jul 21, 2016

bors commented Jul 21, 2016

bors commented Jul 21, 2016

alexcrichton commented Jul 21, 2016

bors commented Jul 21, 2016

bors commented Jul 21, 2016

alexcrichton commented Jul 21, 2016

bors commented Jul 21, 2016

3Hren commented Jul 21, 2016

tamird Jul 22, 2016

Choose a reason for hiding this comment

bors commented Jul 22, 2016

feat: reinterpret `precision` field for strings #34544

feat: reinterpret `precision` field for strings #34544

3Hren commented Jun 29, 2016 •

edited

Loading