Support generating converters for all objects - #1236
Conversation
benquarmby
left a comment
benquarmby
left a comment
There was a problem hiding this comment.
Please consider this WIP for now. Would appreciate some input on the following:
- Can the new
convertersoption ship with just support for JS/TS/Flow? I don't have the expertise required for other languages. - I could use some guidance on how to write tests for this new behavior.
- Any feedback on naming?
|
|
||
| switch (this._jsOptions.converters) { | ||
| case ConvertersOptions.All: | ||
| this.forEachObject("interposing", converter); |
There was a problem hiding this comment.
#naming-is-hard
Is All the right name for this? My assumption is this wont include enums?
There was a problem hiding this comment.
Renamed to AllObjects.
| import { EnumOption } from "../RendererOptions"; | ||
|
|
||
| export enum ConvertersOptions { | ||
| Top = "top", |
There was a problem hiding this comment.
I'd call this TopLevel = "top-level".
|
Yes, this can ship with just JS/TS/Flow. I left a comment on naming. Right now the only easy test you can have is by adding the new option to the |
|
There are some differences in the output of GraphQL tests. Any idea why they happen? |
|
Will rename the Can't explain that diff. Will comb over the code changes again. |
benquarmby
left a comment
benquarmby
left a comment
There was a problem hiding this comment.
Needed to rebase on top of master (and #1221) to fix the diff. All appears to be good now though.
|
|
||
| switch (this._jsOptions.converters) { | ||
| case ConvertersOptions.All: | ||
| this.forEachObject("interposing", converter); |
There was a problem hiding this comment.
Renamed to AllObjects.
| import { EnumOption } from "../RendererOptions"; | ||
|
|
||
| export enum ConvertersOptions { | ||
| TopLevel = "top-level", |
There was a problem hiding this comment.
Renamed to TopLevel.
|
Fantastic, thank you! |
2 participants
Resolves #1235