🐛 Add several missing @return type annotations across the codebase#23559
🐛 Add several missing @return type annotations across the codebase#23559rsimha merged 7 commits intoampproject:masterfrom rsimha:2019-07-26-ReturnAnnotation
@return type annotations across the codebase#23559Conversation
|
The majority of changes in this PR are JSDoc-only. A small number of code changes were needed to satisfy the type checker. Adding several reviewers for visibility. |
jeffkaufman
left a comment
There was a problem hiding this comment.
Thanks for noticing this, and starting the process of fixing it!
I commented on several places where you've used casts where I think annotating the return type would be better.
rsimha
left a comment
There was a problem hiding this comment.
@jeffkaufman Thanks for the patient review. The goal of this PR is to set us on the path to specifying type info wherever required with the minimum of code changes, while also alerting area owners that there is more to be done.
Unfortunately, some of your suggestions could have repercussions that require knowledge of the inner workings of Ads / AMP code. I'd argue that they are better off being made in separate pull requests by those who understand the code the most, so they can be tested in isolation, and trivially reverted if required.
jeffkaufman
left a comment
There was a problem hiding this comment.
@rsimha That's your call to make. I do think addressing the casts soon is valuable, though, because otherwise you can think the type system is protecting you from something when it's not.
Definitely agree. In particularly, I'd love to remove the need for all the casts from |
sparhami
left a comment
There was a problem hiding this comment.
I'm not sure that adding @return {*} TODO: Specify return type is a good change to make. It changes something that could be done as a lint (that could be required when changing a file) into a TODO that might never get done. I'm not sure if our lint setup allows for it, but i have worked in the past with linting setups that require you to make fixes only when a file is modified as part of a change.
zhouyx
left a comment
There was a problem hiding this comment.
analytics and consent related code LGTM
|
@sparhami I've addressed your comments. Some replies below. PTAL.
I don't fully agree with this assessment. There's an advantage to adding all the placeholder
We did this in the past, but abandoned the approach because it made it unduly onerous to make small changes to files with numerous linter errors that would only show up when the file was edited. |
jridgewell
left a comment
There was a problem hiding this comment.
Please file a tracking bug to fix these "TODO: Specify return type" todos.
|
@jridgewell @erwinmombay Feedback addressed. PTAL.
|
|
Tested this locally, Travis is green. Merging before more conflicts show up. Edit: Spoke too soon. One merge conflict snuck in: #23577 (comment). Fix sent out with #23584 |
It turns out there are hundreds of missing
@returnannotations across the AMP codebase, which means closure compiler has incomplete type information during minification.PR highlights:
jsdoc/require-returnslint rule to detect functions that return a value without specifying its type@return {*}to all functions that are missing an annotation (with aTODOto specify the actual type )gulp check-typesNotes:
@return {*}annotations that don't result in type errors can be gradually fixed in follow up PRs@returnannotation will be flagged bygulp lint, and adding a function call where the return type is unspecified will be flagged bygulp check-types