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

feat: support deprecatedOnly option to make deprecated fields optional #1010

Merged
merged 3 commits into from
Mar 26, 2024

Conversation

tufandevrim
Copy link
Contributor

@tufandevrim tufandevrim commented Feb 28, 2024

Add deprecatedOnly option for useOptionals option to make only deprecated fields optional. This PR aims to address the issue #993

@tufandevrim tufandevrim changed the title Add deprecatedOnly option to make deprecated fields optional [feat] support deprecatedOnly option to make deprecated fields optional Feb 28, 2024
@tufandevrim tufandevrim changed the title [feat] support deprecatedOnly option to make deprecated fields optional feat: support deprecatedOnly option to make deprecated fields optional Feb 28, 2024
@tufandevrim
Copy link
Contributor Author

@stephenh thoughts on this PR appreciated. I figured deprecatedOnly in useOptionals config is practical solution but we could also have this as separate option.

Right now in our codebase, we don't use useOptional config which forces developers to set the fields if it is marked as "required" (without optional). Even though we rarely depreciate required fields in our schemas, it is very annoying that we must still set deprecated required fields!

data?: Uint8Array | undefined;
repId: number[];
/** @deprecated */
repState: StateEnum[];
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should this be repState? since it is deprecated? Maybe I am not understanding the purpose of the flag.

Copy link
Owner

Choose a reason for hiding this comment

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

It looks like there is a check to say that repeated fields should not be marked as optional
... @tufandevrim was there a reason to not include this for repeateds?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@paralin @stephenh so sorry for the delay. My original plan was keeping repeated fields out of deprecatedOnly config but it really makes sense to make the repeated fields also optional if they are deprecated. I just updated the code, please review it when you get a chance. Thanks again for the review and feedback.

@stephenh
Copy link
Owner

stephenh commented Mar 6, 2024

Hi @tufandevrim this seems pretty reasonable, see the one question about repeateds, but otherwise this lgtm!

@tufandevrim
Copy link
Contributor Author

Hi @tufandevrim this seems pretty reasonable, see the one question about repeateds, but otherwise this lgtm!

@stephenh yeah that was great feedback and made sense. Just updated the code to make deprecated repeated fields optional. Thanks again

@stephenh
Copy link
Owner

@tufandevrim looks like a minor prettier error in main.ts; can you fix it up? Thanks!

@tufandevrim
Copy link
Contributor Author

@tufandevrim looks like a minor prettier error in main.ts; can you fix it up? Thanks!

Oops! fixed now!

@stephenh
Copy link
Owner

Sweet, thanks @tufandevrim !

@stephenh stephenh merged commit db23004 into stephenh:main Mar 26, 2024
6 checks passed
stephenh pushed a commit that referenced this pull request Mar 26, 2024
# [1.170.0](v1.169.1...v1.170.0) (2024-03-26)

### Features

* support deprecatedOnly option to make deprecated fields optional ([#1010](#1010)) ([db23004](db23004))
@stephenh
Copy link
Owner

🎉 This PR is included in version 1.170.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants