Support custom GTINs

[Narnach]

Support custom GTINs by adding their class to BarcodeValidation::GTIN.gtin_classes

This refactors GTIN handler class lookup so it can be extended without monkeypatching. Developers can add/remove classes in BarcodeValidation::GTIN.gtin_classes to control which class wraps a GTIN.

The use case I need this for is internal dummy GTINs that are longer than any of the standard ones and have additional validation rules besides just length.

The Readme is updated to describe the feature.

[madhums]

@beet could you please also have a look at this?

[beet]

Interested in the use-case for implementing non-standard GTIN lengths, otherwise something like the following would allow us to simply implement any class under BarcodeValidation::GTIN, like BarcodeValidation::GTIN::MyCustomGTIN, without having to do anything else like unshifting constants into an array:

module BarcodeValidation
  module GTIN
    ...snip...

      private

      def class_for_input(input)
        concrete_implementations.find do |klass|
          input.to_s.size == klass::VALID_LENGTH
        end
      end

      def concrete_implementations
        subclasses.select(&:concrete_implementation?)
      end

      def subclasses
        constants.map do |class_name|
          const_get(class_name)
        end
      end

      def concrete_implementation?
        false
      end
    end
  end
end

module BarcodeValidation
  module GTIN
    class Base < BarcodeValidation::DigitSequence
      ...snip...

      def self.concrete_implementation?
        self.const_defined?(:VALID_LENGTH)
      end
    end
  end
end


module BarcodeValidation
  module GTIN
    class CheckDigit < DelegateClass(Digit)
      ...snip...

      def self.concrete_implementation?
        false
      end
    end
  end
end

Could always override #valid? in the custom GTIN implementation for rules like returning true for all inputs of the valid length that start with "123" kinda thing:

module BarcodeValidation
  module GTIN
    class MyCustomGTIN < BarcodeValidation::GTIN::Base
      VALID_LENGTH = 20

      def valid?
        input.start_with?("123") && super
      end
    end
  end
end

[Narnach]

Subclasses like you indicate would be a way to do it, but the downside is that it sort of leaves the ordering of those classes undefined. This is fine if length is the only distinction, but in our case we have specialized GTINs for a subset of data for a particular length, with the default classes being a fallback.

For example: we're running into barcode normalizing behaviors which are different in a few countries we process barcodes from, specifically where supermarket barcodes can embed weight and price information for a weighed piece of fruit.

These normalization rules vary by country and we're looking at moving this logic into our own GTIN classes so we can call #normalize or #normalized? on them, instead of having this code external to the GTINs.

This means we'd like to put our country-specific GTIN classes in the handles? order before the default one so the default can serve as fallback.

We're still in the process of sorting normalization out ourselves, which is why we figured having more control over the loading order like this PR implements allows us to experiment with it. I imagine that implementations of barcode normalization could be interesting for a future PR, if you're interested.

[beet]

Thanks for explaining the concrete use-case.

Is there a way to achieve non-standard lengths without exposing internal implementation details of BarcodeValidation::GTIN?

This feels too tightly coupled: BarcodeValidation::GTIN.gtin_classes.unshift MyCustomGTIN

From the pseudo code above, if we abstract input.to_s.size == klass::VALID_LENGTH out of class_for_input into something like handles?, would that let you achieve what you need by simply defining a class like:

module BarcodeValidation
  module GTIN
    class MyCustomGTIN < BarcodeValidation::GTIN::Base
      VALID_LENGTH = 20

      def self.handles?(input)
        input.start_with?("123") && input.length <= VALID_LENGTH
      end

      def valid?
        input.start_with?("123") && super
      end
    end
  end
end

[Narnach]

I understand the concern about directly exposing the implementation detail of BarcodeValidation::GTIN.gtin_classes and manipulating element order. I've taken the time to think a bit about it.

With some modifications your original self-registering subclasses can probably work very well:

module BarcodeValidation
  module GTIN
    class MyCustomGTIN13 < BarcodeValidation::GTIN::GTIN13
      # We address a subset of GTIN13's range, so ask this class before that one if it handles a GTIN
      prioritize_before BarcodeValidation::GTIN::GTIN13

      # Custom implementations for handles/valid to scope the subset of data this one cares about
      def self.handles?(input)
        input.start_with?("123") && super
      end

      def valid?
        input.start_with?("123") && super
      end
    end
  end
end

The API I'm thinking of for this adds two class-level methods:

• BarcodeValidation::GTIN.prioritized_gtin_classes, an Array which a class is added to the back of whenever a descendant inherits from BarcodeValidation::GTIN::Base. Basically, it behaves like Class#descendants but is available before Ruby 3.1 and is 100% under our control, which is relevant for the next part.
• BarcodeValidation::GTIN::Base.prioritize_before(lower_priority_class) by referencing the other class as argument, you ensure that one is loaded and registered with prioritized_gtin_classes, then the implementation ensures the calling class is no longer behind that class in the list.
• BarcodeValidation::GTIN::Base.abstract_implementation, which removes the calling class from the prioritized_gtin_classes list. It's the same as your "concrete implementation" concept, but in reverse because abstract classes will likely be the exception and thus you don't want to add them everywhere if you don't need to.

I've got some ideas for the implementation, so I'll probably pick it up over the weekend or next week. That gives a small window of opportunity to give feedback on the concepts before they are code 😄

[Narnach]

@beet What do you think of the new implementation?

There are now tests as well, because that's one thing that was also missing in my initial implementation due to it being more of a refactoring that just exposed internals as a side effect.

[Narnach]

Reflection: it feels like managing class lookups is probably a concern that deserves its own class to manage the internal details and expose a clean interface that other classes can use. Right now GTIN::Base is manipulating the internals of GTIN itself, which feels a little icky.

I just wanted to work out an implementation for the proposed public API so if we agree on this, I can refine it instead of wasting my time polishing a dismissed idea 😅

[Narnach]

I've cleaned up the implementation by moving all direct manipulations of the GTIN list into methods in GTIN, rather than doing it from GTIN::Base. I realized that GTIN had no real other concerns that finding + instantiating the correct GTIN class, so to me it makes sense to put the helpers there.

I considered making prioritized_gtin_classes private afterwards, because it's no longer in use outside of GTIN and its tests, but initially found that this made test cleanup harder. Writing this made me realize that this is not a great reason to keep it public, but a great reason to make the tests less reliant on internal implementation details. So I've done that.

What do you think of this implementation?

[beet]

Nice one, thanks @Narnach, very neat!

We almost have a command chain happening now with a means of explicitly positioning a handler before another in the chain, very flexible.

Can look into doing a release next week.

[beet]

@Narnach I tried doing a release, but Rubocop took offence at the build: https://buildkite.com/marketplacer/barcodevalidation/builds/52

Don't know why the PR was green, but the release failed...

[Narnach]

Sorry! I also relied on the PR green checkbox working as intended... 👀

It may be that the checks don't kick in for PRs from external contributors. I've seen this happen on client projects where I started off with PRs from my fork of their project. Github Actions has some options to try and make it work, but for an external platform like buildkite I can imagine they won't do anything unless you're in the organization itself.

That said, I've create a PR to address the issues.