Telepen support

[cpwood]

Addresses #38 .

[hschimke]

I went through and did a basic review over the PR.

I added a few comments for fixes before we can merge.

In addition: I found that the test-suite fails with:

INFO :: Starting test_resources/blackbox/cpp/qrcode-2/12.png
FINE :: could not read at rotation 0: NotFoundException("")
thread 'cpp_qrcode_black_box2_test_case' panicked at src/oned/telepen_reader.rs:217:33:
slice index starts at 1 but ends at 0

Just glancing at it, I believe that the new Telepen reader is faulting when being fed an image with a qr-code in it. Possibly a check there to make sure the out-of-bounds issue does not cause a panic.

Great PR, I'm excited to get this merged, thank you!

[hschimke]

Does telepen_common need to be public here or can it be internal to the crate?

[cpwood]

I wasn't sure whether the raw conversion functions might be of use outside the library, but happy to internalise them.

[hschimke]

I'm fine with it being public in that case, I'm going to move to v0.5 with this PR (and a couple other small fixes I'm rolling up with it), so some changes to the API are totally fine.

[hschimke]

is there a reason why you wrap this in a helper function? I have no objection to it, just curious if I missed something.

[cpwood]

My starting point was to copy the Codabar reader; it's retained from that.

[hschimke]

weird, I wonder why I did that, totally lost to me at this point. probably I copied it from the java codebase for reasons unknown. you can leave it as is and I'll go through and clean them all up at some point in the future.

[cpwood]

Now removed.

[hschimke]

Please just pass this conversion error up with the Result rather than calling .unwrap()

[cpwood]

This one should never have been an unwrap; my mistake!

[hschimke]

please move these tests to the src/oned/telepen_common.rs file, the goal here is to keep the unit tests close to the code they test.

[hschimke]

I didn't select the text correctly, the tests that should be moved are:

#[test]
fn telepen_checksum_test1() {
    let contents = "Hello world!";
    let checksum = TelepenCommon::calculate_checksum(contents);
    assert_eq!('\u{1a}', checksum);
}

#[test]
fn telepen_checksum_test2() {
    let contents = "ABC123456";
    let checksum = TelepenCommon::calculate_checksum(contents);
    assert_eq!('\u{1}', checksum);
}

#[test]
fn telepen_alpha_to_numeric_test() {
    let mut ascii = "'=Siu";
    let mut result = TelepenCommon::ascii_to_numeric(ascii);
    assert_eq!("1234567890", result);

    ascii = "& oe";
    result = TelepenCommon::ascii_to_numeric(ascii);
    assert_eq!("11058474", result);
}

#[test]
fn telepen_numeric_to_ascii_test() {
    let mut numeric = "1234567890";
    let mut result = TelepenCommon::numeric_to_ascii(numeric).unwrap();
    assert_eq!("'=Siu", result);

    numeric = "11058474";
    result = TelepenCommon::numeric_to_ascii(numeric).unwrap();
    assert_eq!("& oe", result);
}

[hschimke]

@cpwood I went through and did a review. Only a few fixes to be handled. The panic when attempting to read a qr-code is the main breaking issue so far.

[cpwood]

@cpwood I went through and did a review. Only a few fixes to be handled. The panic when attempting to read a qr-code is the main breaking issue so far.

No problem. As I mentioned, I'm a rookie Rust developer, so the feedback is gratefully received! Will work on the above and come back to you.

[hschimke]

Sounds good!

A word of warning, even on high-power machines the test suite takes a long time to run if it isn't built in "release" mode, so I always suggest that if you want to run the entire test suite you do so with cargo test --release, the slightly longer build time is worth it in terms of execution time. Obviously not applicable if you're debugging a single test though.

[hschimke]

I poked at it, the out of bounds is fixed with a change to src/oned/telepen_reader.rs on line 217.
I inserted:

if byteLength <= 2 {
            return Err(Exceptions::NOT_FOUND)
        }

but that's probably not the best fix.

[cpwood]

I poked at it, the out of bounds is fixed with a change to src/oned/telepen_reader.rs on line 217. I inserted:

if byteLength <= 2 {
            return Err(Exceptions::NOT_FOUND)
        }

but that's probably not the best fix.

Haha, I've done this:

        // Any Telepen barcode will be longer than two bytes.
        if byteLength < 3 {
            return Err(Exceptions::NOT_FOUND);
        }

The start and end of the barcode are two bytes even before you get to the main content, so it should be 3 bytes at the very least. This should mean we never can go out of bounds anymore.

[hschimke]

Perfect! once you push all your changes let me know and I'll re-auth all the automated tests

[hschimke]

All tests pass, code review looks good, all outstanding concerns fully addressed.

Fully resolves #39

[hschimke]

Oh, thinking about rust style in general, I didn't think about it while I was doing the review but cargo clippy reminded me: the last statement of a function is returned, assuming there is no semicolon after it. It's about 90% of what's in this commit: 4d00d51

It's not required, just a general rust style guide that I try to follow. clippy --fix always suggests the change, which I forget fairly often.

When I did the final prep for the release I noticed that it warned a few times on your functions, and that was really the only thing it complained about.

Great PR, thanks again for your hard work.