-
-
Notifications
You must be signed in to change notification settings - Fork 311
Add support for function paramets without names #218
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
Conversation
README.md
Outdated
| "raw":"string => void", | ||
| "signature":{ | ||
| "arguments":[ | ||
| { "name":"_", "type":{ "name":"string" } } |
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.
What do you think about instead of using '_' setting the field to null? It seems clearer to me that a consumer of react-docgen checks if the name === null instead of a "random" string name === '_'
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.
Hi @danez,
Got the inspiration from a flow-addon for atom (see screenshot). But I agree that null would be a better and cleaner value for working with the data afterwards. Will update the PR.
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.
Edit: getNameOrValue returns a string value, and therefore null would break flow. What do you think @danez?
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 yes seems it is used quite a bit. We could maybe return an empty string? Then !name would work in the resulting documentation.
My intention is just that we should return something else than a non-empty string, because in the '_' case you would not be able to differ if the variable name was _ or empty.
And consumers could still default to _ in the empty case, if the do not display shorthands.
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.
Updated the PR setting the name to an empty string now.
758044d to
dad3800
Compare
src/utils/getNameOrValue.js
Outdated
| return node.name; | ||
| case types.Literal.name: | ||
| return raw ? node.raw : node.value; | ||
| case types.FunctionTypeParam.name: |
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.
Doing this here seems weird to me. I wouldn't expect getNameOrValue to be called with with a type annotation (but who knows...). It's also not quite right, since some FunctionTypeParam do have a name.
Personally I feel it makes more sense to fix this in getFlowType. If the name of the FunctionTypeParam is null, assign an empty string instead. Something like
name: param.node.name ? param.node.name.name || '';
@danez , why does this use getProperty to extract the name at the moment? Shouldn't it directly by getNameOrValue(param.get('name')) or just param.node.name.name?
Am I looking at the wrong syntax tree or am I missing a case here?
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.
Good question, I have no idea :) But seems unnecessary as it can never be computed or be a literal.
Doing name: param.node.name ? param.node.name.name || ''; should be correct in getFlowType unless the tests disagree.
Add support for flow functions with parameters that doesn't have a name. Eg. `value: string => void`. It gives the argument a name equal to an empty string: ''
dad3800 to
e85302d
Compare
|
Hi @fkling, |
|
Great, thank you all! |

Add support for flow functions with parameters that doesn't have a name.
Eg.
value: string => voidandvalue: (string, number) => voidThis kind of functions ins't currently supported by
react-docgen, and it's unable to parse the file.Source: flow