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

Make buildNumber optional #51

Merged
merged 2 commits into from
Jan 5, 2022
Merged

Make buildNumber optional #51

merged 2 commits into from
Jan 5, 2022

Conversation

aslakhellesoy
Copy link
Contributor

@aslakhellesoy aslakhellesoy commented Jan 4, 2022

Description

Make buildNumber an optional field.

Motivation & context

The TypeScript definition of CiEnvironment incorrectly declared the field as mandatory, but at runtime it may be optional.

Type of change

  • Breaking change (will cause existing functionality to not
    work as expected)

Checklist:

  • I have read the CONTRIBUTING document.
  • My code follows the code style of this project.
  • I have added tests to cover my changes.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have added an entry to the "Unreleased" section of the CHANGELOG, linking to this pull request.

@mpkorstanje
Copy link
Contributor

So what about messages? It is required there.

@aslakhellesoy
Copy link
Contributor Author

aslakhellesoy commented Jan 5, 2022

When we added the field in 17.0.0 it was a mistake to define this field as mandatory.

It was a mistake because consumers have to handle previous versions of messages where the field would always be absent. As a general rule, we should always declare new fields as not required in order to be able to handle older versions.

So I propose:

  • We make a new messages release where the field is optional. It's debatable whether it is a bugfix or a breaking change - I'm leaning towards bugfix in this case.
  • I update this PR to make it optional here too.

It's a real shame jsonschema2pojo doesn't generate Optional getters for non-required fields. There is an open PR for it: joelittlejohn/jsonschema2pojo#1057. We could fork and release our own maven package, or we could abandon it altogether and use custom code generation like we do for TypeScript, Go and Ruby. I think maybe the latter.

WDYT?

@aurelien-reeves
Copy link
Contributor

aurelien-reeves commented Jan 5, 2022

Well, I don't know with java. But in the schema, and in js and ruby and go implementation, the build number is not mandatory in messages

@aslakhellesoy
Copy link
Contributor Author

You're right @aurelien-reeves - it's not required. What made you think it was required @mpkorstanje?

@mpkorstanje
Copy link
Contributor

Ah. My bad. That should have been a question, not an assertion.

So we're good.

@mpkorstanje
Copy link
Contributor

It's a real shame jsonschema2pojo doesn't generate Optional getters for non-required fields. There is an open PR for it: joelittlejohn/jsonschema2pojo#1057. We could fork and release our own maven package, or we could abandon it altogether and use custom code generation like we do for TypeScript, Go and Ruby. I think maybe the latter.

One thing at a time.

@mpkorstanje mpkorstanje merged commit 95f5677 into main Jan 5, 2022
@mpkorstanje mpkorstanje deleted the make-build-number-optional branch January 5, 2022 11:35
@aslakhellesoy
Copy link
Contributor Author

@mpkorstanje I wasn't quite ready with this PR, ref my suggestion above to make buildNumber optional. Please don't release this until I have fixed that.

@mpkorstanje
Copy link
Contributor

Not sure what you mean. I did see buildNumber?: string so I assume you made it optional.

@aslakhellesoy
Copy link
Contributor Author

You're right - ignore me. Getting my wires crossed.

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