-
Notifications
You must be signed in to change notification settings - Fork 521
Add support for global include policy. #284
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
base: develop
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @jasperblues, thanks for your contribution.
Are you still interested in this change?
I am not sure if this will be accepted, but I believe it might be useful to discuss it with the current maintainer. In the meanwhile I did a first review in your code. If you are still interested on it, please submit an updated version of your PR. If accepted, additional changes may be required before the final approval.
In addition, you will need to rebase your branch with the current development branch.
"remap-istanbul": "^0.7.0", | ||
"sinon": "^1.17.4", | ||
"sinon-chai": "^2.8.0", | ||
"ts-node": "^8.3.0", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This file and the package-lock.json are not required and should be removed from this PR.
*/ | ||
includePolicy?: Include; | ||
|
||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please remove the double spaces.
@@ -0,0 +1,10 @@ | |||
export enum Include { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should discuss the name for this enum. This one is not clear enough.
return keys.filter((it: string) => object[it] !== null); | ||
} else { | ||
throw new Error(`Unsupported Include policy ${include}`); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Suggested changes:
if (include === Include.DEFAULT) {
return keys;
}
if (include === Include.NON_NULL) {
return keys.filter((it: string) => object[it] !== null);
}
throw new Error(`Unsupported Include policy ${include}`);
The variable names include
and it
are not clear enough. Please consider changing them later.
|
||
class User { | ||
|
||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please remove double spaces.
I'm no longer personally interested, but happy to update if it looks useful for community. |
…per Blues for the "include policy", and feedback from the pull request. typestack#284
…per Blues for the "include policy", and feedback from the pull request. typestack#284
…ies if they have null values. Re-implements pull request typestack#284, incorporating changes by Jasper Blues for the "include policy", and feedback from the pull request. typestack#284
We require the ability to exclude null fields, therefore
Similar to the Jackson serializer for Java, add support for a global include policy: https://fasterxml.github.io/jackson-annotations/javadoc/2.7/com/fasterxml/jackson/annotation/JsonInclude.Include.html
Current possible values are:
[DEFAULT, NON_NULL]