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

Establish consistent usage of 'desc' in public models #234

Merged
merged 3 commits into from
Feb 5, 2024
Merged

Conversation

samlown
Copy link
Collaborator

@samlown samlown commented Jan 22, 2024

  • Removing usage of "desc" property name in favour of "description" which is used more widely throughout GOBL and is consistent with JSON Schema definitions.

@samlown samlown requested a review from cavalle January 22, 2024 12:05
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.

Changes LGTM (just left one comment). Regarding impact on modules/providers, if I'm not mistaken all our modules use either Go's gobl or gobl.ruby to access GOBL documents. That means that we would need to:

  1. Prepare a change in gobl.ruby updating to the latest GOBL schema and adding backwards compatibility for the old desc name.
  2. Update all the modules (or, at least, all that access to any description field, e.g. PDF, Plemsi, SAT-MX…) to use the latest version of gobl or gobl.ruby to ensure they'll be able to process documents with both desc and description fields.

Copy link
Contributor

Choose a reason for hiding this comment

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

No custom UnmarshalJSON for this struct as well?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think my though process here was that we've not seen anybody use this field, so its not worth the extra code... have you seen any exceptions?

Copy link
Contributor

Choose a reason for hiding this comment

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

No, I have not. There's no need.

@samlown samlown merged commit 313e3cf into main Feb 5, 2024
2 checks passed
@samlown samlown deleted the migrate-desc branch February 5, 2024 12:15
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