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.

Sign up for GitHub

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

Jump to bottom

Feat/optional elements support #64

Closed
wants to merge 9 commits into from
Closed

Feat/optional elements support #64

wants to merge 9 commits into from
+25 −6

Conversation

danomrc
Copy link

@danomrc danomrc commented Jan 20, 2025 •
edited
Loading

Motivation

Which issue does this fix? Fixes #39

If no issue exists, what is the fix or new feature? Were there any reasons to fix/implement things that are not obvious?

Adds the ability to mark 'Not Required' elements as optional in generated models. By default the ability is disabled so as to not be a breaking change. Adds a flag/option to enable it.

Didn't add much to the README but happy to contribute more examples there.

Open to a better name than optionalElements for the option!

Aware these changes will be made redundant by PR #61 but the idea to have optionals enabled by a flag is one that could be carried forward.

Checklist

  • Code follows coding conventions held in this repo
  • Automated tests have been added
  • Tests are passing
  • Docs have been updated (if applicable)
  • Temporary settings (e.g. variables used during development and testing) have been reverted to defaults

How to test

If manual testing is required, what are the steps?

  1. Pass the optionalElements flag into the cli and confirm that models are generated with properties marked as optional where expected.
  2. Using the generateModelsAsync function, confirm that setting optionalElements to true causes the outputted models to have properties marked as optional where expected.

@danomrc danomrc marked this pull request as ready for review January 20, 2025 13:19
@danomrc danomrc requested review from IvanKiral, Enngage and a team as code owners January 20, 2025 13:19
@Enngage Enngage self-assigned this Jan 20, 2025
@Enngage
Copy link
Member

Enngage commented Jan 20, 2025

Hi @danomrc, thank you very much for your interest and taking the time to work on this. However, this implementation doesn't solve the issue as it marks the entire element object as optional which is incorrect as only value should be marked that way. However, this is something that cannot be solved with model generator, rather we need to adjust the types within the https://github.com/kontent-ai/delivery-sdk-js first to allow proper configuration of required / optional elements. We are planning on making these improvements in the upcoming months.

The implementation might be even a bit different as things are shaken a bit by the preview mode as well.

For that reason, I'm sorry, but I have to close this PR as it isn't the right fit for merging.

@Enngage Enngage closed this Jan 20, 2025
@danomrc
Copy link
Author

danomrc commented Jan 20, 2025 •
edited
Loading

Thanks for taking a look, and the response - yeah, it hadn't crystallised that the whole element isn't coming in as undefined

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@IvanKiral IvanKiral Awaiting requested review from IvanKiral IvanKiral is a code owner

@Enngage Enngage Awaiting requested review from Enngage Enngage is a code owner

Assignees

@Enngage Enngage

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Support Optional Elements
2 participants
@danomrc @Enngage