Skip to content
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

refactor(deprecation)!: remove legacy support for array arguments #625

Merged
merged 6 commits into from May 16, 2019

Conversation

schmidt-sebastian
Copy link
Contributor

BREAKING CHANGE: This removes the ability to call getAll() and new FieldPath() with an array as its first argument instead of calling it with a list of arguments.

@googlebot googlebot added the cla: yes This human has signed the Contributor License Agreement. label May 15, 2019
Copy link
Contributor

@mikelehen mikelehen left a comment

Choose a reason for hiding this comment

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

I think this looks good but maybe we need to update a couple of the jsdoc parameter definitions to match?

const elements: string[] = Array.isArray(segments[0])
? ((segments[0] as unknown) as string[])
: segments;
if (Array.isArray(segments[0])) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Remove string[] from the param type definition above?

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. Also removed "undocumented" from the PR description...

@@ -433,30 +433,23 @@ export function parseGetAllArguments(
let documents: DocumentReference[];
Copy link
Contributor

Choose a reason for hiding this comment

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

I can't comment on unchanged code, but does the getAll() parameter definition need to be updated somehow?

   * @param {Array.<DocumentReference|ReadOptions>} documentRefsOrReadOptions
   * The `DocumentReferences` to receive, followed by an optional field
   * mask.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes. I fixed the JSDoc as well.

@schmidt-sebastian schmidt-sebastian changed the title breaking!: Remove undocumented legacy support for array arguments breaking!: Remove legacy support for array arguments May 15, 2019
@bcoe bcoe changed the title breaking!: Remove legacy support for array arguments refactor(deprecation)!: remove legacy support for array arguments May 15, 2019
@codecov
Copy link

codecov bot commented May 16, 2019

Codecov Report

Merging #625 into master will decrease coverage by 0.02%.
The diff coverage is 71.42%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #625      +/-   ##
==========================================
- Coverage    61.6%   61.57%   -0.03%     
==========================================
  Files          21       21              
  Lines        3422     3420       -2     
  Branches      459      459              
==========================================
- Hits         2108     2106       -2     
  Misses       1252     1252              
  Partials       62       62
Impacted Files Coverage Δ
dev/src/index.ts 95.19% <ø> (ø) ⬆️
dev/src/path.ts 97.39% <71.42%> (-0.87%) ⬇️
dev/src/transaction.ts 92.95% <71.42%> (-2.94%) ⬇️
dev/src/watch.ts 97.68% <0%> (+1.15%) ⬆️

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 9474e3f...667d715. Read the comment docs.

@codecov
Copy link

codecov bot commented May 16, 2019

Codecov Report

❗ No coverage uploaded for pull request base (master@9474e3f). Click here to learn what that means.
The diff coverage is 71.42%.

Impacted file tree graph

@@            Coverage Diff            @@
##             master     #625   +/-   ##
=========================================
  Coverage          ?   61.32%           
=========================================
  Files             ?       21           
  Lines             ?     3405           
  Branches          ?      460           
=========================================
  Hits              ?     2088           
  Misses            ?     1254           
  Partials          ?       63
Impacted Files Coverage Δ
dev/src/index.ts 95.01% <ø> (ø)
dev/src/path.ts 97.39% <71.42%> (ø)
dev/src/transaction.ts 92.95% <71.42%> (ø)

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 9474e3f...667d715. Read the comment docs.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes This human has signed the Contributor License Agreement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants