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

Format optional types with ? instead of [] #538

Merged
merged 3 commits into from
Nov 18, 2016
Merged

Format optional types with ? instead of [] #538

merged 3 commits into from
Nov 18, 2016

Conversation

tmcw
Copy link
Member

@tmcw tmcw commented Sep 9, 2016

Fixes #509

Formats optionals with ? instead of [], to avoid confusion with array types.

2016-09-09 at 11 18 am

@arv
Copy link
Contributor

arv commented Sep 9, 2016

I like this better. Thanks.

I'm still wondering if bar(a: number = 2) is doable?

@ikokostya
Copy link
Contributor

Why ?a instead a?:

bar(a?)
bar(a?: number)

?

@tmcw
Copy link
Member Author

tmcw commented Sep 17, 2016

Why ?a instead a?:

This is matching the Flow syntax ( https://flowtype.org/docs/nullable-types.html ), and also aligns with the rest of the types: the optionality of the parameter is part of its type, and the pattern is param: type, so the ? should go on the type side, not the parameter side.

@ikokostya
Copy link
Contributor

ikokostya commented Sep 29, 2016

This is matching the Flow syntax ( https://flowtype.org/docs/nullable-types.html ), and also aligns with the rest of the types: the optionality of the parameter is part of its type

From https://flowtype.org/docs/builtins.html#null-and-void

Optional object properties and optional function parameters have the type T|void, for some type T.

Maybe types have the type T|void|null for some type T.

As I understand param?: type and param: ?type have different semantics. Also in ES6 null value doesn't trigger default value for optional parameter, e.g. bar: ?number is incorrect syntax for the following jsdoc:

/**
 * @param {number} [bar=1]
 */
function foo(bar = 1) {}

@tmcw
Copy link
Member Author

tmcw commented Nov 17, 2016

Yep, you're right @ikokostya - thx for checking :) Working on an updated version now.

@tmcw
Copy link
Member Author

tmcw commented Nov 17, 2016

2016-11-17 at 6 17 pm

This is the one corner case right now... since we pass only the param type - not the param - to formatType in the theme, the formatType method doesn't have access to a default value so can't include it in the type. I've changed the (default foo) style to = foo but you still have the reiterating bar? = foo syntax in the theme. mulling over whether zapping that ? is worth a more in-depth change to how we use formatType...

@tmcw
Copy link
Member Author

tmcw commented Nov 18, 2016

I'm comfortable with keeping this corner case for now, after exploring alternatives. Those are:

  • Copy the default property from the param to its type property. This would be the least invasive change, but would change our JSON output and make our types not 100% the same as doctrine's, which is not great.
  • Make format_type accept params and props rather than their types. This gets messy, since it is recursive to handle nested types.

Happy to rethink this in the future or accept a PR of an improvement on this one!

@tmcw tmcw merged commit 2ac4d04 into master Nov 18, 2016
@tmcw tmcw deleted the optional-format branch November 18, 2016 14:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants