Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Fix for Issue#343 #353

Merged
merged 8 commits into from
Nov 8, 2018
Merged

Fix for Issue#343 #353

merged 8 commits into from
Nov 8, 2018

Conversation

muraliQlogic
Copy link
Contributor

Fixes #343

  • Tests and linter pass
  • Code coverage does not decrease (if any source code was changed)
  • Appropriate docs were updated (if necessary)

@googlebot googlebot added the cla: yes This human has signed the Contributor License Agreement. label Nov 4, 2018
@sduskis
Copy link
Contributor

sduskis commented Nov 5, 2018

@muraliQlogic: The requirement is to have a check that requires a family with a qualifier. The qualifier could be the empty string. This PR doesn't look right.

Copy link
Contributor

@sduskis sduskis left a comment

Choose a reason for hiding this comment

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

Please add a test to ensure that an empty qualifier passes the chunk transformer. Please revert everything else.

@sduskis
Copy link
Contributor

sduskis commented Nov 6, 2018

fam":"" is fine, but "fam":[null] is not ok. I believe the if statement checks for "fam":[null] rather than "fam":"". We should add a unit test to confirm that. @muraliQlogic, can you please change the the chunktransformer.js test for those requirements?

@codecov
Copy link

codecov bot commented Nov 6, 2018

Codecov Report

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

Impacted file tree graph

@@           Coverage Diff           @@
##             master   #353   +/-   ##
=======================================
  Coverage          ?   100%           
=======================================
  Files             ?     10           
  Lines             ?   1287           
  Branches          ?      0           
=======================================
  Hits              ?   1287           
  Misses            ?      0           
  Partials          ?      0
Impacted Files Coverage Δ
src/chunktransformer.js 100% <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 a8f156d...ca14a39. Read the comment docs.

@muraliQlogic
Copy link
Contributor Author

@sduskis : I modified if condition to check for null or undefined qualifier.

@sduskis sduskis added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Nov 6, 2018
@kokoro-team kokoro-team removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Nov 6, 2018
@sduskis
Copy link
Contributor

sduskis commented Nov 6, 2018

@muraliQlogic: Does ChunkTransformer. validateNewRow also need the check for null or undefined?

@sduskis sduskis merged commit e47d80a into googleapis:master Nov 8, 2018
# for free to join this conversation on GitHub. Already have an account? # 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.

5 participants