Skip to content
This repository has been archived by the owner on Feb 26, 2024. It is now read-only.

Add support for collapsing parameter properties with default values. #738

Merged
merged 9 commits into from
May 25, 2018

Conversation

bowenni
Copy link
Contributor

@bowenni bowenni commented May 8, 2018

Fixes #650

One side-effect of this change is that it adds type annotations to function parameters with default values which fixes the implicity any errors.

@bowenni
Copy link
Contributor Author

bowenni commented May 8, 2018

#731 is closely related to this issue (rest parameters instead of parameters with default values).

@rkirov rkirov self-requested a review May 10, 2018 00:19
@@ -162,6 +162,19 @@ public void visit(NodeTraversal t, Node n, Node parent) {
maybeSetInlineTypeExpression(parent, n, bestJSDocInfo, true);
}
break;
// If a DEVAULE_VALUE is in a PARAM_LIST we type annotate its first child which is the
// actual parameter.
case DEFAULT_VALUE:
Copy link
Contributor

Choose a reason for hiding this comment

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

default values can occur in ES6 destructuring too:

let {x = 0} = {};

Are sure this makes sense for those cases?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I didn't know this syntax before.

Added a test case. It seems fine because the parent node here is not a parameter list.

@@ -308,12 +308,6 @@ public void visit(NodeTraversal t, Node n, Node parent) {
if (!nodeAfterDefault.isName()) {
continue;
}
// It appears that adding ACCESS_MODIFIERs to Default params do not come out though
// the CodeGenerator, thus not safe to remove the declaration.
// TODO(rado): fix in emitting code and remove this line.
Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks, for cleaning up my TODOs!

@@ -5,7 +5,7 @@ class Klass {

/**
* @param {number} n
* @param {Array<?>=} list
Copy link
Contributor

Choose a reason for hiding this comment

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

Why delete the '='? I looked around and it appears closure allows one to write this with or without the '=', but the one with '=' is more common.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Reverted.

String paramName = nodeAfterDefault.getString();
@Nullable
JSTypeExpression paramType =
constructorJsDoc == null ? null : constructorJsDoc.getParameterType(paramName);
// The class member declaration on "this" will not have "=" in the JSDoc most of the time. Therefore we need to remove the "=" from the JSDoc of the parameter declaration so we can compare the types and collapse the parameter property.
Copy link
Contributor

Choose a reason for hiding this comment

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

comment goes over 100+ chars

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.
gradle googleJavaFormat stopped working for me.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Now I look at that, the code passed on travis so maybe our formatter setup is not working.

@bowenni bowenni merged commit 89dd1dd into angular:master May 25, 2018
@bowenni bowenni deleted the fb4 branch May 25, 2018 18:56
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Gents doesn't emit parameter properties when the parameters have default values
3 participants