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

fix #4784 (standalone) #4901

Closed
wants to merge 3 commits into from
Closed

fix #4784 (standalone) #4901

wants to merge 3 commits into from

Conversation

OfekShilon
Copy link
Contributor

@OfekShilon OfekShilon commented Feb 13, 2021

Closes #4784 and #4816

Per maintainers' request - a PR containing just a single fix for 4784.

@codecov
Copy link

codecov bot commented Feb 13, 2021

Codecov Report

Merging #4901 (c6ccf74) into master (79c3d3e) will decrease coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #4901      +/-   ##
==========================================
- Coverage   99.46%   99.46%   -0.01%     
==========================================
  Files          75       75              
  Lines       14707    14705       -2     
==========================================
- Hits        14629    14627       -2     
  Misses         78       78              
Impacted Files Coverage Δ
src/assign.c 99.70% <ø> (-0.01%) ⬇️
R/data.table.R 99.94% <100.00%> (ø)

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 79c3d3e...c6ccf74. Read the comment docs.

@OfekShilon
Copy link
Contributor Author

Will close also #4816

@OfekShilon
Copy link
Contributor Author

I actually think the reduction in test coverage is a false positive, and anyway is tiny. Will this be accepted for merge?

@OfekShilon
Copy link
Contributor Author

The reported reduction in test-coverage is only due to (justified) deletion of 2 covered lines: https://codecov.freshdesk.com/support/solutions/articles/43000634519-unexpected-drop-in-project-coverage-for-pull-request
Is there still a reason not to merge this?
@MichaelChirico

@OfekShilon
Copy link
Contributor Author

Hey. Is there anything more needed to have this merged? Thanks.

@OfekShilon
Copy link
Contributor Author

@jangorecki @MichaelChirico Is there anything unclear, or otherwise missing, to merge this fix?

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