Add alternate hint ('#') #503

richard-uk1 · 2021-06-06T22:39:29Z

Also make formatting match core::fmt.

This changes the current behavior, so some thought should be given before accepting. I expect it will break some tests: I'm not sure how to run them all locally so I will rely on CI to find them.

Closes #491

Also make formatting match core::fmt.

richard-uk1 · 2021-06-06T23:19:14Z

I might need a little bit of help/advice getting this through the CI.

book/src/hints.md

Urhengulas · 2021-06-07T09:30:29Z

decoder/src/frame.rs

@@ -162,14 +171,23 @@ fn format_args_real(
        buf: &mut String,
    ) -> Result<(), fmt::Error> {
        match hint {
-            Some(DisplayHint::NoHint { zero_pad }) => write!(buf, "{:#01$}", x, zero_pad)?,
-            Some(DisplayHint::Binary { zero_pad }) => write!(buf, "{:#01$b}", x, zero_pad)?,
+            Some(DisplayHint::NoHint { zero_pad }) => write!(buf, "{:01$}", x, zero_pad)?,


Note to future: The body of fn format_u128 and fn format_i128 is pretty much the same. Refactor!

I did play around with doing this, but you would need something that is generic over u128/i128, or you could refactor it into a macro (since the actual language tokens are the same but the types are different).

parser/src/lib.rs

Urhengulas · 2021-06-07T09:36:31Z

I'm not sure how to run them all locally so I will rely on CI to find them.

You can run the ci, by invoking cargo xtask test-all in the repository root (see cargo xtask --help for more options).

decoder/src/lib.rs

Urhengulas · 2021-06-07T09:48:32Z

The snapshot tests fail because it is missing all the 0xes and 0bs, e.g.:

-0.000024 INFO hex     [0x48, 0x65, 0x7f, 0x6c, 0x6c, 0x6f]
-0.000025 INFO HEX     [0x48, 0x65, 0x7F, 0x6C, 0x6C, 0x6F]
-0.000026 INFO binary  [0b1001000, 0b1100101, 0b1111111, 0b1101100, 0b1101100, 0b1101111]
+0.000024 INFO hex     [48, 65, 7f, 6c, 6c, 6f]
+0.000025 INFO HEX     [48, 65, 7F, 6C, 6C, 6F]
+0.000026 INFO binary  [1001000, 1100101, 1111111, 1101100, 1101100, 1101111]

You can run only the snapshot tests with cargo xtask test-snapshot.

We should probably duplicate all the failing ones and have one with # and one without.

richard-uk1 · 2021-06-07T12:17:45Z

Thanks for the review, I'll have another go at getting it working.

Is there a way to overwrite the .out files in firmware/qemu? I'm adding in new things into the .rs which means everything changes. Mighty be easier to read as a diff.

Also add the ability to overwrite output files in snapshot test using xtask. It is sometimes quicker to overwrite the output and then compare it, rather than altering it manually.

richard-uk1 · 2021-06-07T14:19:22Z

OK ready for review again. Let me know if you want me to take back out the --overwrite flag from xtask, but it is very useful!

richard-uk1 · 2021-06-07T14:23:47Z

Could you let me know how to fix the last CI error?

Urhengulas · 2021-06-07T14:24:06Z

Let me know if you want me to take back out the --overwrite flag from xtask, but it is very useful!

I agree that it seems useful, but can you please send it as a separate PR?

Urhengulas · 2021-06-07T14:24:30Z

Could you let me know how to fix the last CI error?

Will check :)

Urhengulas · 2021-06-07T14:31:24Z

Could you let me know how to fix the last CI error?

The failing backwards-compatability check means, that the output changed compared to the decoder version 0.2.0.
(failing ci: https://github.com/knurling-rs/defmt/pull/503/checks?check_run_id=2764884042#step:7:174)

This might mean this change qualifies as breaking, but I will check with the others.

Urhengulas · 2021-06-07T22:44:11Z

Could you let me know how to fix the last CI error?

The failing backwards-compatability check means, that the output changed compared to the decoder version 0.2.0.
(failing ci: https://github.com/knurling-rs/defmt/pull/503/checks?check_run_id=2764884042#step:7:174)

This might mean this change qualifies as breaking, but I will check with the others.

Since we don't guarantee backwards-compatability this isn't a problem, we just need to update these tests.

Urhengulas · 2021-06-08T09:18:04Z

Since we don't guarantee backwards-compatability this isn't a problem, we just need to update these tests.

So the solution is to install qemu-run from a commit including your changes. I got it to work locally, now it is the question how to teach the CI how to do this 🤔

Urhengulas · 2021-06-08T09:32:29Z

book/src/hints.md

+leading zeros.  This matches [`core::fmt` behavior](https://play.rust-lang.org/?version=stable&mode=debug&edition=2018&gist=b11809759f975e266251f7968e542756). No further
+customization like padding is supported (at the moment).


No further customization like padding is supported (at the moment).

Aren't the leading zeroes a kind of padding?

Hmm good point, would you like me to change this? Maybe to No further customization is supported (at the moment).?

(I guess this should have changed with your last PR, but good that we caught it now ;D)

Urhengulas · 2021-06-08T10:52:34Z

Can you please apply following change which should make CI pass?

diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml
index 6956a1f..6cae683 100644
--- a/.github/workflows/ci.yml
+++ b/.github/workflows/ci.yml
@@ -124,8 +124,7 @@ jobs:
       - name: Run QEMU snapshot tests
         run: cargo xtask test-snapshot
       - name: install decoder v0.2.0
-        working-directory: firmware/qemu
-        run: cargo install --debug --git https://github.com/knurling-rs/defmt --rev v0.2.0-with-qemu-run-ignore-version qemu-run
+        run: cargo install --debug --path qemu-run
       - name: Backward compatibility check against decoder v0.2.0
         env:
           CARGO_TARGET_THUMBV7M_NONE_EABI_RUNNER: qemu-run

Urhengulas

Just the text-nit and then we are good to merge :)

Urhengulas · 2021-06-11T13:10:30Z

bors d+ (now you should be able to merge it via bors yourself)

bors · 2021-06-11T13:10:31Z

✌️ derekdreery can now approve this pull request. To approve and merge a pull request, simply reply with bors r+. More detailed instructions are available here.

richard-uk1 · 2021-06-11T16:59:44Z

bors r+

bors · 2021-06-11T17:05:08Z

Build succeeded:

ci

richard-uk1 added 2 commits June 6, 2021 23:37

Add alternate hint ('#')

0c8aefc

Also make formatting match core::fmt.

Fix CI errors.

e334aa6

Urhengulas requested changes Jun 7, 2021

View reviewed changes

Urhengulas reviewed Jun 7, 2021

View reviewed changes

decoder/src/lib.rs Outdated Show resolved Hide resolved

richard-uk1 added 2 commits June 7, 2021 15:14

Fix up errors/tests.

32232cc

Also add the ability to overwrite output files in snapshot test using xtask. It is sometimes quicker to overwrite the output and then compare it, rather than altering it manually.

Add back in github fix.

4c19d20

richard-uk1 force-pushed the hex_match_core_fmt branch from c5bdc36 to 4c19d20 Compare June 7, 2021 14:16

Revert changes to xtask

fe7edbe

richard-uk1 mentioned this pull request Jun 7, 2021

Xtask qemu overwrite #504

Merged

Urhengulas approved these changes Jun 8, 2021

View reviewed changes

Make requested CI change.

de63fc0

Urhengulas approved these changes Jun 11, 2021

View reviewed changes

Clarify documentation.

dbd915f

bors bot merged commit 19f92c5 into knurling-rs:main Jun 11, 2021

Urhengulas mentioned this pull request Jun 11, 2021

Backwards-compatability tests #506

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add alternate hint ('#') #503

Add alternate hint ('#') #503

richard-uk1 commented Jun 6, 2021 •

edited

Loading

richard-uk1 commented Jun 6, 2021

Urhengulas Jun 7, 2021

richard-uk1 Jun 7, 2021

Urhengulas commented Jun 7, 2021

Urhengulas commented Jun 7, 2021 •

edited

Loading

richard-uk1 commented Jun 7, 2021 •

edited

Loading

richard-uk1 commented Jun 7, 2021

richard-uk1 commented Jun 7, 2021

Urhengulas commented Jun 7, 2021

Urhengulas commented Jun 7, 2021

Urhengulas commented Jun 7, 2021

Urhengulas commented Jun 7, 2021

Urhengulas commented Jun 8, 2021 •

edited

Loading

Urhengulas Jun 8, 2021

richard-uk1 Jun 10, 2021

Urhengulas Jun 11, 2021

Urhengulas Jun 11, 2021

Urhengulas commented Jun 8, 2021

Urhengulas left a comment

Urhengulas commented Jun 11, 2021

bors bot commented Jun 11, 2021

richard-uk1 commented Jun 11, 2021 •

edited

Loading

bors bot commented Jun 11, 2021

		leading zeros. This matches [`core::fmt` behavior](https://play.rust-lang.org/?version=stable&mode=debug&edition=2018&gist=b11809759f975e266251f7968e542756). No further
		customization like padding is supported (at the moment).

Add alternate hint ('#') #503

Add alternate hint ('#') #503

Conversation

richard-uk1 commented Jun 6, 2021 • edited Loading

richard-uk1 commented Jun 6, 2021

Urhengulas Jun 7, 2021

Choose a reason for hiding this comment

richard-uk1 Jun 7, 2021

Choose a reason for hiding this comment

Urhengulas commented Jun 7, 2021

Urhengulas commented Jun 7, 2021 • edited Loading

richard-uk1 commented Jun 7, 2021 • edited Loading

richard-uk1 commented Jun 7, 2021

richard-uk1 commented Jun 7, 2021

Urhengulas commented Jun 7, 2021

Urhengulas commented Jun 7, 2021

Urhengulas commented Jun 7, 2021

Urhengulas commented Jun 7, 2021

Urhengulas commented Jun 8, 2021 • edited Loading

Urhengulas Jun 8, 2021

Choose a reason for hiding this comment

richard-uk1 Jun 10, 2021

Choose a reason for hiding this comment

Urhengulas Jun 11, 2021

Choose a reason for hiding this comment

Urhengulas Jun 11, 2021

Choose a reason for hiding this comment

Urhengulas commented Jun 8, 2021

Urhengulas left a comment

Choose a reason for hiding this comment

Urhengulas commented Jun 11, 2021

bors bot commented Jun 11, 2021

richard-uk1 commented Jun 11, 2021 • edited Loading

bors bot commented Jun 11, 2021

richard-uk1 commented Jun 6, 2021 •

edited

Loading

Urhengulas commented Jun 7, 2021 •

edited

Loading

richard-uk1 commented Jun 7, 2021 •

edited

Loading

Urhengulas commented Jun 8, 2021 •

edited

Loading

richard-uk1 commented Jun 11, 2021 •

edited

Loading