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

refactor: remove * from application identifiers #74

Merged
merged 3 commits into from
Feb 18, 2025

Conversation

superskip
Copy link
Collaborator

@superskip superskip commented Feb 13, 2025

Asterisks appearing in the application identifiers were not properly handled. Summary of the issues:

  • During parsing of a datamatrix string, suppose the next application identifier matches the pattern 326*. The existing implementation will accept any value in place of *, but in reality only a number in the range 0-5 is a valid match.
  • Different application identifiers allow different replacement values for *, for instance in the pattern 391* the * may be any number in the range 0-9.
  • To look up an application identifier after the parsing is complete, a provided string representing an application identifier is compared against the enum values to see if there is any matching entries. If there is no match an error is thrown, otherwise the string is translated into the matching enum value. This translation is a problem if the string matched an application identifier containing a *, because a queried key 3261 for instance will then be translated into 326*, losing the last digit, which is needed when doing the lookup.

Summary of the summary: the existing implementation doesn't specify which values * may correspond to, and information is lost during query key validation.

The solution presented in this pull request is to remove asterisks from the system rather than trying make it work. The main drawback with this approach is that it considerably increases the size of enum class GS1Elements (from 200 to 527 elements). The main benefit it that it fixes the above problems with next to no changes in the parsing logic.

@vgarciabnz
Copy link
Member

Apart from the failing test, the public API validation is going to fail as well because this PR changes the public API of the repo.

You have to run the command ./gradlew apiDump to regenerate the api file and include it in the PR (check this part in the README). You can execute the command in Intellij UI as well.

If you think this class must be internal, maybe it is good chance to make it internal, although it might force you to make more classes internal. Anyway, the api file must be updated to reflect the changes.

@vgarciabnz
Copy link
Member

Do not forget to increase the version number in the build.gradle.kts file. It has to be done manually.

@superskip
Copy link
Collaborator Author

superskip commented Feb 14, 2025

If you think this class must be internal, maybe it is good chance to make it internal, although it might force you to make more classes internal.

@vgarciabnz I don't think I have the required kotlin skills to make that sort of refactor 😅 If you or someone else would like to give it a try, maybe you can create a branch from this branch or something.. (pushing directly to this branch should probably also be fine)

@vgarciabnz
Copy link
Member

No worries, that's fine. I will leave @jbee to approve it as I don't control the logic of the parser so much.

Copy link
Collaborator

@jbee jbee left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't really know much about the GS1 stuff but from the PR description the change makes sense to me.

@superskip superskip merged commit 2853700 into main Feb 18, 2025
2 checks passed
@superskip superskip deleted the d2ExtractDataMatrixValue branch February 28, 2025 10:14
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.

3 participants