Dependencies/Tests/Deprecation/Validation/Chore/Typos

[monheimx9]

Hi Yatekii, This is my first time contributing to an open source project besides my own, I'm probably missing some organisation and planning :^) Since we started working on it almost at the same time by coincidence with @jacg I also suggest that you could take a look if the direction I'm taking is aligning with what you want too At the same times I was also working on the clippy warnings too, there are commits with the same code

I started to write some unit testing too, and also began to write integration tests, I'm planning to make more of them in the future

I'm working on a CLI tool that use that library to replace the python script that I previously made, I'll be using your library in the small company were I work

On the lib.rs file I think it's not necessary to call twice node::Text as follow:

Text::new("")
.add(svg::node::Text::new(line))

This should be sufficient

Text::new(line)

While exploring the Todo's I also made that change, is it what you intended?

  // TODO: validate QR IBAN / QRID matches.
        match &options.reference {
            Reference::Qrr(x) => x.validate_qriid(&options.account)?,
            Reference::Scor(s) => s.validate_iid(&options.account)?,
            Reference::None => {}
        }

In the Esr library I took the liberty of deprecating the "try_new()" function and replacing it with two new functions, one with checksum, the other without.

Do you think this was a good idea? I wanted to give the user the possibility of generating a valid Esr number directly from the library. I'm thinking about doing the same for the SCOR reference in the future

Anyway, feel free to point out any changes that you want I wish you a good day

[jacg]

I started to write some unit testing too, and also began to write integration tests, I'm planning to make more of them in the future

I have some (very ugly, but working in principle) tests for checking that the data encoded in the QR code are as expected:

1. Verify output of QRBill::qr_data()
2. Roundtrip:
   • write QR code to PNG file
   • read file
   • decode using bardecoder
3. Same as the previous one but keeping the image in memory without writing to file. This one doesn't work yet.

They use rstest to create an example fixture which is used in the three aforementioned varieties of test. Eventually I'd like to use proptest to generate multiple examples. (@Yatekii how would you feel about adding these to dev-dependencies?)

Let me know if you are doing anything along these lines, so we can avoid duplicating effort, or share ideas.

Incidentally, it was pretty easy to find examples which produced test failures when going via the PNG file. I haven't had the time to figure out which component in the cycle was responsible for the failure, so it might just be that one of the dev-dependencies used only in the test is the weak like, in which case the failures are spurious, but maybe there's a more fundamental problem with the real code itself.

[monheimx9]

I have some (very ugly, but working in principle) tests for checking that the data encoded in the QR code are as expected:

1. Verify output of QRBill::qr_data()

2. Roundtrip:

   • write QR code to PNG file
   • read file
   • decode using bardecoder
3. Same as the previous one but keeping the image in memory without writing to file. This one doesn't work yet.

@jacg Great idea, I've only worked a little bit with "Image Buffer" and don't feel very confident in it, if you're cooking something I would gladly see it

They use rstest to create an example fixture which is used in the three aforementioned varieties of test. Eventually I'd like to use proptest to generate multiple examples. (Yatekii how would you feel about adding these to dev-dependencies?)

Let me know if you are doing anything along these lines, so we can avoid duplicating effort, or share ideas.

Sure, at this time I'm working on a separate CLI tool and I'm building a validator/builder for Swico S1 Syntax for the extra_info field See https://github.com/Yatekii/qrbill/issues/13 For now it's only WIP in my local repo, but depending on how it goes I could add it here instead

If @Yatekii is okay using adding rstest and proptest I would gladly change my current unit tests and learn how to use rstest and proptest

[Yatekii]

Sure, feel free to use all the amazing crates :) We should also set up some CI pipeline :)

[monheimx9]

Sure, feel free to use all the amazing crates :) We should also set up some CI pipeline :)

Then here are my commits :^)

It fixes a mistake I did on the qriid validation match on the qr reference, also added the rstest dev-dependency You will see that I made public use of NaiveDate and CountryCode to avoid to import them again in any crate that depend on qrbill

I've introduced "kind of" a breaking change, since you made a new() function on the StructuredAddress struc, I've made the fields private to force error detection on creation

Also, how do we deal with this typo ? ^_^

#[derive(Debug)]
pub enum Address {
    Cobined(CombinedAddress),
    Structured(StructuredAddress),
}

Do we keep Cobined instead of Combined :D ?

If everything is ok for you I think this pull request is long enough I should have squashed a few commits and should have rebased before pushing my branch, I'm sorry I'm still on the beginner side on the Git etiquette

My next PR's will be cleaner

Regarding the CI pipeline I've never done any, but if I can be of any help feel free to ask I also think it could be nice to add one on this project

[jacg]

Sorry about my radio silence. Hopefully I'll find some time to contribute a bit in the next few days.

GHA OK for the CI?

[Yatekii]

Thanks a lot folks!

re: Typo: I would just fix it :) I don't think this crate sees much usage apart from the three of us and I rather fix an API that is an internal detail (and not on the wire) than keep it broken forever :)

re: CI: yeah GHA is great :) Maybe I find some time tomorrow :)