Support nullable #535
Support nullable #535
Conversation
- Add handling for enum, anyOf, and oneOf - So long as the order of normalizers is maintained, no handling for const is necessary - Add additional tests
- Update log message - Support schema.const in case transforms are reordered in the future
- Apply new prettier styles
mdmower
changed the title
|
@bcherny - Would you be willing to review? |
- Introduce warning() to output console warnings - Add warning for nullable const, where const value is not null - Move nullable transform before const->enum transform so that potential warning about nullable const can display - Remove anyOf and oneOf handling since JSON spec should only constrain possibilities, not expand (allowance made for enum and const to expand types but they show warnings when this happens). - Avoid pushing 'null' onto available types if it already exists.
|
|
||
| if (schema.const !== undefined) { | ||
| if (schema.const !== null) { | ||
| warning('normalizer', 'const should be set to null when schema is nullable', schema) |
There was a problem hiding this comment.
I understand why you added warning as its a good addition, but unless I'm reading the utils incorrectly it will always log if this conditional is hit. I don't see any other logs in this file, just errors being thrown.
The only location error is imported is in the cli.ts file and that means the programmatic version will always log if this is hit, when you are actually under the hood fixing the issue vs. logging a properly formated error during a cli command.
(see my comment on the recommended change in utils.tsa).
| if (!process.env.VERBOSE) { | ||
| return console.warn(messages) | ||
| } | ||
| console.warn(getStyledTextForLogging('yellow')?.('warning'), ...messages) |
There was a problem hiding this comment.
This will always log. If you look at the log function below it actually returns void when its NOT verbose.
Since this is just a warning I recommend the following change:
| if (!process.env.VERBOSE) { | |
| return console.warn(messages) | |
| } | |
| console.warn(getStyledTextForLogging('yellow')?.('warning'), ...messages) | |
| if (!process.env.VERBOSE) { | |
| return | |
| } | |
| console.warn(getStyledTextForLogging('yellow')?.('warning'), ...messages) |
There was a problem hiding this comment.
This was written to mimic
json-schema-to-typescript/src/utils.ts
Lines 205 to 210 in 6adcad9
I don't pretend to understand that choice and it's not my aim to question it. I'm just following precedence here.
There was a problem hiding this comment.
I figured, but I believe that is only done in the cli not meant for run time usage as I don't really want console.error or console.warn being called when I use the library programatically.
|
@bcherny - I still hold hope that this PR could move forward. Is there anything I can do to improve its chances of merge? Maybe you have an opinion on zrosenbauer's review above? The logging was added based on this comment and jackfrankland's subsequent reply. |
nullable: trueto includenullas a possible type for any of the following schema types:const,enum,type.constis not set tonullbut schema is markednullable: trueenumdoes not containnullbut schema is markednullable: trueThis is a continuation of #522 by @jackfrankland to finish adding support for
nullable: truein schemas.