-
-
Notifications
You must be signed in to change notification settings - Fork 75
New: Mark optional parameters (fixes #186) #187
Conversation
LGTM |
Are there other cases where we need this: interface foo {
bar?: string
} or classes class foo {
public bar?: string
} |
We would need everything to be able to print the AST back to source. This was just what I encountered but to avoid a bunch of PRs to extend that if I can try to lookup and fix other cases. Apart from those two, properties in type object literals come to mind as well but I guess there can be even more. |
c5e3e1a
to
d52e2de
Compare
LGTM |
I think I have gotten all cases, however the SyntaxKinds does not align perfectly with the spec so if you think of any other case to test please alert me. |
I think this is good for now. I will likely refactor this a bit in a future PR to have consistency around the |
Blast, this was out of date with master and has caused an issue with the new optional params. I will submit a fix. I thought github repo was configure to block situations like that |
Purely a test output that needs updating, no functional regression |
I can rebase the PRs on each other if you prefer. Just tell me which and in what order. |
We'll just rebase each PR against master ahead of merging |
@JamesHenry Another possibility is re-trigger the Travis PR build before merging a PR if you had merged a few other PRs before getting to that one- Travis will run the PR against latest master. |
Good point, @platinumazure! Thanks |
I choose to avoid storing
optional: false
to avoid a giant mess of failing tests, as undefined is false the use case still works.