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

Unit updates and compatibility with UN/ECE codes #223

Merged
merged 5 commits into from
Nov 30, 2023
Merged

Conversation

samlown
Copy link
Collaborator

@samlown samlown commented Nov 30, 2023

  • Adds a few more "frequently used" unit keys for Mexico
  • Brings support for being able to use either a unit key or UN/ECE unit code in the unit value (not entirely sure yet if this is genius or stupid.)

@samlown samlown requested a review from cavalle November 30, 2023 15:26
Copy link
Contributor

@cavalle cavalle left a comment

Choose a reason for hiding this comment

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

Still not in love with making the unit field polymorphic:

  • It feels hacky.
  • It would be the only field in GOBL with two different formats/meanings.
  • We are not guaranteeing it will be one of the valid UNECE codes (only that it has 2 or three uppercase letters)
  • We can always extend the list of units for common UNECE units (which, IMO, should also include newton meters (UN) simply because it could become a common mistake for XUN)
  • We could always use extensions if we really need them.

@samlown
Copy link
Collaborator Author

samlown commented Nov 30, 2023

@cavalle I hear you, but I'm not convinced on the alternate paths at the moment. Also bare in mind that the previous version allowed any key in the unit field. You'll see in the commit there that some of the tests in Mexico were using an invalid unit, which wasn't great either.

While it feels a bit unusual, the JSON Schema is sound. Its just a string for which you can use a constant, or a code. The unit isn't used for decision making, which lessons the requirements, and I think its useful to be able to fall back on standard codes if the customer requires it.

I'll commit for the time being so we can fix some of the issues in Mexico.

@samlown samlown merged commit b3b111c into main Nov 30, 2023
2 checks passed
@samlown samlown deleted the unit-unece-compat branch November 30, 2023 21:45
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.

2 participants