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.

Sign up for GitHub

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

Jump to bottom

refactor(typescript): enable noImplicitAny #553

Merged
merged 4 commits into from
Feb 25, 2019
Merged

refactor(typescript): enable noImplicitAny #553

merged 4 commits into from
Feb 25, 2019

Conversation

schmidt-sebastian
Copy link
Contributor

@schmidt-sebastian schmidt-sebastian commented Feb 22, 2019 •
edited
Loading

This removes no-implicit-any from the SDK.

Fixes: #525

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

codecov bot commented Feb 22, 2019

Codecov Report

Merging #553 into master will increase coverage by <.01%.
The diff coverage is 97.87%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #553      +/-   ##
==========================================
+ Coverage   95.85%   95.86%   +<.01%     
==========================================
  Files          24       24              
  Lines        1932     1933       +1     
  Branches      168      168              
==========================================
+ Hits         1852     1853       +1     
  Misses         57       57              
  Partials       23       23
Impacted Files Coverage Δ
dev/src/field-value.ts 93.75% <ø> (ø) ⬆️
dev/src/types.ts 100% <ø> (ø) ⬆️
dev/src/logger.ts 100% <ø> (ø) ⬆️
dev/src/geo-point.ts 91.66% <ø> (ø) ⬆️
dev/src/document-change.ts 100% <ø> (ø) ⬆️
dev/src/path.ts 97.61% <ø> (ø) ⬆️
dev/src/timestamp.ts 100% <100%> (ø) ⬆️
dev/src/watch.ts 99.04% <100%> (ø) ⬆️
dev/src/write-batch.ts 100% <100%> (ø) ⬆️
dev/src/serializer.ts 96.59% <100%> (ø) ⬆️
... and 4 more

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 b1f6c75...8f92627. Read the comment docs.

@codecov
Copy link

codecov bot commented Feb 22, 2019 •
edited
Loading

Codecov Report

Merging #553 into master will decrease coverage by <.01%.
The diff coverage is 98.07%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #553      +/-   ##
==========================================
- Coverage   95.85%   95.85%   -0.01%     
==========================================
  Files          24       24              
  Lines        1932     1931       -1     
  Branches      168      168              
==========================================
- Hits         1852     1851       -1     
  Misses         57       57              
  Partials       23       23
Impacted Files Coverage Δ
dev/src/field-value.ts 93.75% <ø> (ø) ⬆️
dev/src/types.ts 100% <ø> (ø) ⬆️
dev/src/logger.ts 100% <ø> (ø) ⬆️
dev/src/geo-point.ts 91.66% <ø> (ø) ⬆️
dev/src/document-change.ts 100% <ø> (ø) ⬆️
dev/src/path.ts 97.61% <ø> (ø) ⬆️
dev/src/timestamp.ts 100% <100%> (ø) ⬆️
dev/src/watch.ts 99.04% <100%> (ø) ⬆️
dev/src/write-batch.ts 100% <100%> (ø) ⬆️
dev/src/serializer.ts 96.51% <100%> (-0.08%) ⬇️
... and 4 more

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 8b5d087...ea2ddfa. Read the comment docs.

@schmidt-sebastian schmidt-sebastian changed the title WIP: No implicit any No implicit any Feb 25, 2019
mikelehen
mikelehen approved these changes Feb 25, 2019
View reviewed changes
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.

LGTM

dev/src/index.ts Outdated
* @param readTime An ISO-8601 formatted string indicating the time this
* document was read.
* @param encoding 'json' to indicate the format of `document` and `readTime`.
* @returns The QueryDocumentSnapshot with the document's data.
*/
Copy link
Contributor

Choose a reason for hiding this comment

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

Hrm. Duplicating all the docs for each overload like this seems a bit verbose and likely to get out-of-sync over time. Was there a particular motivation? I don't feel strongly, so feel free to leave as-is.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was mostly intended to get IntelliJ to stop warning me about missing docs for some of the arguments (since the overloads use different argument names). Since I wasn't 100% convinced of this when I did it, I removed this part of the changes. I did keep the overloads though.

@JustinBeckwith JustinBeckwith changed the title No implicit any refactor(typescript): enable noImplicitAny Feb 25, 2019
@schmidt-sebastian schmidt-sebastian merged commit 257abb9 into master Feb 25, 2019
@schmidt-sebastian schmidt-sebastian deleted the mrschmidt-noimplicitany branch February 25, 2019 21:44
@schmidt-sebastian schmidt-sebastian mentioned this pull request Feb 25, 2019
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mikelehen mikelehen mikelehen approved these changes

Assignees

@schmidt-sebastian schmidt-sebastian

Labels
cla: yes This human has signed the Contributor License Agreement.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@schmidt-sebastian @mikelehen @googlebot