-
-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
Fix Warning in ArgumentRenderer #1667 #1671
Conversation
Codecov Report
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the pull request!
src/loaders/utils/getProps.ts
Outdated
@@ -94,7 +94,7 @@ export default function getProps(doc: DocumentationObject, filepath?: string): R | |||
allTags as TagProps, | |||
JS_DOC_METHOD_RETURN_TAG_SYNONYMS | |||
) as TagParamObject[]; | |||
const returns = method.returns || returnTags[0]; | |||
const returns = (method.returns || returnTags[0]) && { ...method.returns, ...returnTags[0] }; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like this isn't different from the previous version, unless I'm missing something (in this case we should make it more clear).
We're returning method.returns
if it's truthy, or returnTags[0]
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hmm... we need to make it more clear. in my case, method.returns
have this object:
{
description: 'return a Boolean Value',
type: { name: 'boolean' }
}
and returnTags[0]
have this object:
{
title: 'returns',
description: 'return a Boolean Value',
type: { type: 'NameExpression', name: 'boolean' }
}
method.returns
will have occurred DoctrineError
when it does not have { type: 'NameExpression' }
.
so I figured that merging both results would result in the intended object shape.
But I'm also sure I don't understand enough about this logic.
What should the correct object shape of results
be?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Now I'm even more confused. So now we're overwriting anything in method.returns
with returnTags[0]
but previously method.returns
had higher priority.
If method.returns
is missing the type, why not add it there? Something like this:
const getReturns = ({method, tags}) => {
if (method.returns) {
return {
...method.returns,
type: { type: 'NameExpression', ...method.returns.type }
}
}
const returnTags = getMergedTag(
tags as TagProps,
JS_DOC_METHOD_RETURN_TAG_SYNONYMS
) as TagParamObject[];
return returnTags[0];
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
previously
method.returns
had higher priority.
I got it. I’ll add a change on the method.returns
side.
@sapegin please review me. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🦄
🎉 This PR is included in version 11.0.11 🎉 The release is available on: Your semantic-release bot 📦🚀 |
Resolved
I checked the behavior of the component of the original issue after I changed the
name
to optional and found that the following error occurred.What I have found in my research is that in the above cases,
type.type
in thereturns
is lost ingetProps
in this case.returns
should haveNameExpression
intype.type
in my opinion.