bwipp / postscriptbarcode

Barcode Writer in Pure PostScript
https://bwipp.terryburton.co.uk
MIT License
460 stars 64 forks source link

For fixed-width inputs, do not automatically calculate a missing check digit, unless actually told to #263

Open terryburton opened 6 months ago

terryburton commented 6 months ago

We've focused a lot on reducing human error in recent releases, e.g. with the GS1 AI syntax linting.

One source of error that I'm aware of is users accidentally missing a digit in the input for a symbology that has fixed-width input (such as EAN-13) and not realising that a check digit has been automatically added.

Given the example of EAN-13, the current behaviour is:

Data Options Result Note
9520123456788 Validate check digit and generate
952012345678 Calculate check digit and generate Check digit calculation occurs automatically

In future, a new calculatecheck option will be introduced, and the behaviour will be changed to:

Data Options Result Note
9520123456788 Validate check digit and generate
952012345678 Error: EAN-13 must be 13 digits, or 12 with calculatecheck option Check digit calculation is no longer automatic
952012345678 calculatecheck Calculate the check digit (as told) and generate

There's a decision to be made over whether or not to silently ignore the new calculatecheck option when a full width input is already provided, option (1):

Data Options Result Note
9520123456788 calculatecheck Validate check digit and generate Ignore being asked to generate a check digit if it is already present

versus option (2):

Data Options Result Note
9520123456788 calculatecheck Error: EAN-13 must be 12 digits with calculatecheck option

My preference is option (2) since this covers the case that a user wants their check digit to be calculated but erroneously enters 13 digits (and the check calculation happens to pass).

The above is an "API change" that maximises the utility of the check digit that is built into these fixed-width symbologies (and their derivatives):

terryburton commented 6 months ago

Soliciting opinions on the above from those who have to handle fallout from downstream or cross-testing:

@gitlost @metafloor @semireg @adamchainz

semireg commented 6 months ago

This looks good to me and makes sense. Curious to see if @metafloor changes the API to match, or if defaults will be maintained along with a new negated boolean (skip-calculatecheck) to access this new behavior. Thanks for checking with us!

gitlost commented 6 months ago

Hi, option (2) looks good to me.

Zint should probably do the same, it's permissive at the moment like current BWIPP.

On Fri, Feb 16, 2024 at 3:01 PM Caylan @.***> wrote:

This looks good to me and makes sense. Curious to see if @metafloor https://github.com/metafloor changes the API to match, or if defaults will be maintained along with a new negated boolean (skip-calculatecheck) to access this new behavior. Thanks for checking with us!

— Reply to this email directly, view it on GitHub https://github.com/bwipp/postscriptbarcode/issues/263#issuecomment-1948542349, or unsubscribe https://github.com/notifications/unsubscribe-auth/AADVVPVESTT44GYDWVK5XFTYT5YERAVCNFSM6AAAAABDMA4QYCVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTSNBYGU2DEMZUHE . You are receiving this because you were mentioned.Message ID: @.***>

terryburton commented 6 months ago

Curious to see if @metafloor changes the API to match, or if defaults will be maintained along with a new negated boolean (skip-calculatecheck) to access this new behavior. Thanks for checking with us!

@semireg For API back compatibility, we could introduce a deprecated BWIPP option calculatecheckifmissing, which frontends could initially set unconditionally to restore the previous behaviour. This would allow for each frontend to transition at its own pace without need to create some tricky shim over the BWIPP inputs to fake up the old behaviour. I would be loathed to document this option (it's not something that I think general users should reply upon), and I would be tempted to remove it at some point in the not too distant future.

metafloor commented 6 months ago

I am traveling this weekend, but would like to add logging to the bwip-js public API to get a sense of how much current usage relies on automatic checkdigit calculation. This is a potentially breaking change for folks who rely on the public API for commercial use.

metafloor commented 5 months ago

I added logging that just records text length for the list of symbols above. The service provider (heroku) restarts the service every 24 hours, and the numbers below represent about 14 hours uptime. Aside from lots of spurious text lengths that would have generated errors, here is sample usage for the public api:

ean13={ ... "12":2934,"13":63885 ... }
ean8={"8":4}
upca={ ... "10":146,"11":18507,"12":8483,"13":54,"14":16}
upce={"5":3,"6":1,"7":3,"8":281,"10":1}
isbn={}
ismn={}
issn={}
databarlimited={"18":19}
databaronni={}
databarstackedomni={}
databarstacked={}
databartruncated={}
sscc18={}
ean14={"17":2,"18":67}
itf14={"13":162,"14":124}
identcode={}
leitcode={}
pzn={}
code32={}

The pairs of digits are "length-of-text":count. As it stands now, there are a significant number of calls that expect the check digit to be calculated automatically. Unfortunately, I have no idea how many distinct users that represents.

My thinking is that when this change hits, bwip-js will add additional logic to the ean, upc and itf14 symbols to automatically add the calculatecheck option if the text length is one digit short. That's an easy fix to maintain backward compatibility for the existing users.

semireg commented 5 months ago

Nice work on the stats.

By add the calculatecheck option do you mean calculatecheck will default to true in bwip-js but defaults to false in bwip? Or is this new behavior only going to be in the public API to not break things?

metafloor commented 5 months ago

@semireg: I have not decided public api vs base bwip-js, but am leaning towards just the public api. My thinking is that when this change hits, bwip-js will release as a major version change, since it is a breaking api change. That will keep most npm users tied to the previous version until they explicitly choose to make the upgrade. The public api users, on the other hand, likely have minimal visibility into these types of changes

terryburton commented 5 months ago

My current plan is as follows:

Data Options Result Note
9520123456788 Validate check digit and generate
952012345678 Error: EAN-13 must be 13 digits, or 12 with calculatecheck option Check digit calculation is no longer automatic
952012345678 calculatecheck Calculate the check digit (as told) and generate
9520123456788 calculatecheck Error: EAN-13 must be 12 digits with calculatecheck option
952012345678 calculatecheckifmissing Calculate the check digit (as told) and generate. Deprecated option, for back compat. Can exist alongside calculatecheck in which case calculatecheck is ignored.
9520123456788 calculatecheckifmissing Validate check digit and generate. "

So BWIP-JS et al. can just unconditionally inject calculatecheckifmissing (in these and any other symbologyies, where it is unknown and will therefore be ignored) to obtain the existing behaviour.

Does that work? This way the transition is to simply remove calculatecheckifmissing at the appropriate time.

metafloor commented 5 months ago

@terryburton: For bwip-js, the logic around injecting calculatecheck or unconditionally adding calculatecheckifmissing is a wash. My biggest concern is properly documenting the temporary nature of the work around, and encouraging the users to upgrade their code. That is one limitation of github and especially npm that is tricky to work with.

terryburton commented 5 months ago

My biggest concern is properly documenting the temporary nature of the work around, and encouraging the users to upgrade their code. That is one limitation of github and especially npm that is tricky to work with.

Agreed.