Upgrade nom dependency to 7.1.3

[ahartel]

No description provided.

[jedireza]

Thanks for creating a PR!! 🦾

Looks like a couple checks failed. Rustfmt should be easy to resolve. Overall the changes seem reasonable, but I also wasn't able to review in depth yet. I'm hoping that @jhwgh1968 might be able to take a look. I'm also hopeful our test cases cover enough to give confidence.

[jhwgh1968]

Thanks indeed @ahartel!

Looking over this quickly, I appreciate a lot of the fixes and lints. In the end, I have only one key question

Why was ToString trait impl changed to Display?

I am not sure exactly why I chose ToString but I think it had to do with trait compatibility. Note also the impl of From on every "string reference" type.

There is also behavior around Debug which would be affected by Display and I am not quite sure about.

Does this change facilitate something specific besides those .to_string() removals?

[ahartel]

Thanks for taking a look!

Why was ToString trait impl changed to Display?

The lint that clippy gave was this one.
It basically says that the ToString trait gets automatically implemented for any type which implements Display. From that perspective it should probably be fine?!

[ahartel]

I fixed the formatting and one more clippy lint which I had missed.
But it seems that the failing test is the test record::raw_tests::verify_display which is testing the stringification of the type RawRecordHeader. But I didn't change the Display impl of that type at all.
The test is failing both on the beta 1.83 and on the stable 1.82 toolchain. And it is also only failing sometimes. Which seems to imply that this has something to do with non-guaranteed order of the formatter?!

My proposal would be to change the assert to a "contains" assert. Any thoughts?

[jhwgh1968]

The lint that clippy gave was this one.

Ah, I did not know about this lint when I wrote the code! I guess that makes sense to change to a Display attribute then. If they want the output I originally had, it can be Debug.

Based on this, please do a "major bump" of the crate version -- so in this case, a second digit. Changing this trait behavior can indeed have consequences.

My proposal would be to change the assert to a "contains" assert. Any thoughts?

That test was written to ensure that the format would be exactly correct for what was input, because this would be used for outputting it to data streams. So I am worried about contains.

I suspect this test has always been 50-50 due to a coin flip on the HashMap which contains a random factor. I would actually rather have then be collected into a list and sorted for consistent output -- both for the test and for the end user.

Hopefully that would not be too much trouble?

Thanks for your work on this!

[jedireza]

But it seems that the failing test is the test record::raw_tests::verify_display which is testing the stringification of the type RawRecordHeader.

Funny I never noticed or don't remember this was flaky. There's a proposed fix for that over here: #42 (review)

[ahartel]

Based on this, please do a "major bump" of the crate version -- so in this case, a second digit. Changing this trait behavior can indeed have consequences.

Done

[ahartel]

Funny I never noticed or don't remember this was flaky. There's a proposed fix for that over here: #42 (review)

Thanks for pointing that out. Didn't change the test in this case.

[jhwgh1968]

Hmm, I see.

While I'd like to see a complete output test, I will accept that other fix for now. Hopefully once that merges this can too.

[jedireza]

Published as v0.4.0