Skip to content

emit full jsdoc for function types #54

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

Merged
merged 1 commit into from
Feb 10, 2016
Merged

emit full jsdoc for function types #54

merged 1 commit into from
Feb 10, 2016

Conversation

evmar
Copy link
Contributor

@evmar evmar commented Feb 9, 2016

Rather than emitting a type per parameter, emit a jsdoc
before the function declaration. This allows us to mark optional
arguments properly with the = suffix. Note the changed "optional.ts"
test; it now verifies that Closure accepts a missing argument when
that argument was missing before.

Fixes #43.

@@ -1 +1 @@
var fn3 = (/** number */ a) => 12;
var fn3 = (a) => 12;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks like we lost the comment? But it appears in sickle/arrow_fn.ts, so I am very confused.

@rkirov
Copy link
Contributor

rkirov commented Feb 9, 2016

Should we file a bug at https://github.com/google/closure-compiler/issues for function f(/** number= */) not working? Sounds like a bug to me.

@evmar
Copy link
Contributor Author

evmar commented Feb 9, 2016

function f(/** number= */) is expected to not work. Something about how = is not part of the type but actually part of the @param declaration. E.g. I think you can't declare a variable with a type that includes = in it.

@evmar
Copy link
Contributor Author

evmar commented Feb 9, 2016

Note that the weird output with the duplicated comment is due to
microsoft/TypeScript#6982

@evmar evmar assigned mprobst and unassigned rkirov Feb 9, 2016
@evmar
Copy link
Contributor Author

evmar commented Feb 9, 2016

Reassigning to Martin because Rado is out today.

@evmar
Copy link
Contributor Author

evmar commented Feb 9, 2016

Travis is failing due to the compile timing out. :~( I guess I gotta refactor things to run the compilations separately. The compile succeeds locally though it takes a while.

Rather than emitting a type per parameter, emit a jsdoc
before the function declaration.  This allows us to mark optional
arguments properly with the = suffix.  Note the changed "optional.ts"
test; it now verifies that Closure accepts a missing argument when
that argument was missing before.

Fixes #43.
@evmar
Copy link
Contributor Author

evmar commented Feb 9, 2016

With the timeout "fix" in place, this is now ready for review.

@rkirov
Copy link
Contributor

rkirov commented Feb 10, 2016

I don't see how you fixed the timeout issue. But LGTM, since it looks fixed.

@rkirov rkirov added the LGTM label Feb 10, 2016
@rkirov rkirov assigned rkirov and unassigned mprobst Feb 10, 2016
@rkirov
Copy link
Contributor

rkirov commented Feb 10, 2016

I thought T= is sugar for T | undefined, but really it is marker for the function type. The more your know ...

@evmar evmar merged commit 16a0f69 into angular:master Feb 10, 2016
@evmar evmar deleted the optional branch February 10, 2016 18:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Optional arguments need to propagate into Closure type
4 participants