Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix: re-expose GTIN#check_digit as public method #28

Merged
merged 1 commit into from
Aug 30, 2022

Conversation

Narnach
Copy link
Contributor

@Narnach Narnach commented Mar 10, 2022

Restore BarcodeValiation::GTIN#check_digit as public method (broken in v2.3.0).

PR #3 changed the internals of BarcodeValidation::GTIN to support different GTIN formats and to allow conversion between them. While this is a very nice feature, it unfortunately made the previously public method GTIN#check_digit (which was added via BarcodeValidation::Mixin) private.

In v2.2.0 we relied on this in our tests to fix the check digit of randomly generated barcodes. v2.3.0 makes both check_digit and expected_check_digit private, so it does not publicly expose any of the tools we'd need for our purposes.

The simplest fix seemed to make check_digit public, which restores the API that was available in v2.2.0. I've added a spec that calls the method and its validation as a means to ensure it remains public.

The readme still references check_digit as a public method, which reinforces that it was probably made private by accident.

(Note that I read the commit template, but it seems copy/pasted from another project that has interface tests and authentication. I did try to keep the first paragraph of my PR suitable for a changelog, because I'm not sure if that is relevant or not.)

PR marketplacer#3 changed the internals of BarcodeValidation::GTIN to support
different GTIN formats and to allow conversion between them. While this is
a _very_ nice feature, it unfortunately made the previously public method
`GTIN#check_digit` (which was added via BarcodeValidation::Mixin) private.

In v2.2.0 we relied on this in our tests to fix the check digit of randomly
generated barcodes. v2.3.0 makes both `check_digit` and `expected_check_digit`
private, so it does not publicly expose any of the tools we'd need for
our purposes.

The simplest fix seemed to make `check_digit` public, which restores the
API that was available in v2.2.0. I've added a spec that calls the
method and its validation as a means to ensure it remains public.

Ref: marketplacer#3
@wvengen
Copy link

wvengen commented Apr 21, 2022

Thanks for the PR! I'm bumping into this as well. @marketplacer if you find the time to merge this, very welcome :)

@madhums
Copy link

madhums commented Aug 24, 2022

This would be very helpful for me too! @nrw505 @beet could you please have a look?

@beet
Copy link
Contributor

beet commented Aug 25, 2022

Making the check digit private was a conscious decision to treat it as an internal implementation detail of the GTIN objects' #valid? public predicate method:

def valid?
  valid_length == length && check_digit.valid?
end

Could you walk us through how you're using the check digit directly?

@Narnach
Copy link
Contributor Author

Narnach commented Aug 25, 2022

I can see how hiding implementation details is a good reason to shrink your public API. In this case, though, check digits are part of the barcode domain and hiding it means you're hiding a useful domain object. In our case we relied on it, so we did not have to re-implement it ourselves (which seems wasteful).

In addition to reading and validating existing barcodes, we have two reasons for generating them:

  • When we generate barcodes for internal use
  • When we generate random unique barcodes during testing, and we would prefer them to be valid

One example of how we use the public check digit would be this:

# Takes a barcode string and if needed corrects the check digit to make it valid.
#
# @param [String] bc A barcode that may or may not have a valid check digit.
# @return [String] A barcode that has a valid check digit.
def repair_check_digit(bc)
  bc = bc.dup
  bc[-1] = BarcodeValidation.scan(bc).check_digit.expected.to_s
  bc
end

@beet
Copy link
Contributor

beet commented Aug 28, 2022

@Narnach Nice one, thanks for the explanation, makes perfect sense.

@madhums
Copy link

madhums commented Aug 29, 2022

Ah great! Can we merge and do a release? @beet :)

@beet beet merged commit 79767ac into marketplacer:main Aug 30, 2022
@beet
Copy link
Contributor

beet commented Aug 30, 2022

Just released Release v2.5.0 · marketplacer/barcodevalidation

@Narnach
Copy link
Contributor Author

Narnach commented Sep 1, 2022

Nice, thank you @beet!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants