Skip to content

Fix documentation for include and select #601

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
Aug 6, 2018
Merged

Fix documentation for include and select #601

merged 1 commit into from
Aug 6, 2018

Conversation

MatrixFrog
Copy link

These both can accept either a string, or an array of strings

Copy link
Contributor

@flovilmart flovilmart left a comment

Choose a reason for hiding this comment

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

Why isn’t the change reflected in the type of the keys param?

@MatrixFrog
Copy link
Author

Ah, thanks, I totally missed that you're using both type annotations in comments, and TS annotations

These both can accept either a string, or an array of strings
@codecov
Copy link

codecov bot commented Jul 12, 2018

Codecov Report

Merging #601 into master will not change coverage.
The diff coverage is 100%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #601   +/-   ##
=======================================
  Coverage   84.57%   84.57%           
=======================================
  Files          48       48           
  Lines        4039     4039           
  Branches      911      911           
=======================================
  Hits         3416     3416           
  Misses        623      623
Impacted Files Coverage Δ
src/ParseQuery.js 93.8% <100%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2640391...c94ee64. Read the comment docs.

Copy link
Contributor

@flovilmart flovilmart left a comment

Choose a reason for hiding this comment

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

I believe there’s still a mismatch, the thing is, the keys are spread, So it’s already accurate, a single string, an array of string are valid values.

@MatrixFrog
Copy link
Author

I'm not sure what you want it to say. The way it's written, you could pass multiple arrays of strings, like include(['foo'], ['foo.bar', 'bar']) though I don't know if you want to document that as being supported.

I was following the docs at https://www.typescriptlang.org/docs/handbook/functions.html#rest-parameters which seem to imply that the type of a ... param is always an array. But I'm not sure if you're actually using TypeScript or Flow, and it's possible they disagree slightly on this.

@flovilmart
Copy link
Contributor

Right, that’s this part that’s confusing ...String|Array<string> as it should be read:

(spread of (string or arrays of strings) ) instead of ((spread of string) or (array of strings))

In doubt that’s confusing and even if it’s supported it’s rather make the documentation unambiguous even if it means sacrificing a ‘feature’ or corner case. What do you think?

In the end, it supports only arrays, which can be strings of arrays of strings. So we should remove the spread operator from the jsdocs as it’s confusing (me) an perhaps others.

What do you think?

@MatrixFrog
Copy link
Author

Okay so you can basically call it in two ways: You can call it as though the @param annotation in the JSDoc was {...String} or you can just pass a single array as though it was {Array<String>}. (Other ways of calling it are possible but rare enough that we don't want to document them.)

I'm not sure how to represent that in the JSDoc. Maybe just what I have in the PR currently, except with parens: {(...String)|Array<string>}

@flovilmart
Copy link
Contributor

The parens may help with the readability.

@flovilmart flovilmart merged commit bb8955f into parse-community:master Aug 6, 2018
@MatrixFrog
Copy link
Author

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants