search

PIC-IRIS / PH5

Library of PH5 clients, apis, and utilities
Other
15 stars 9 forks source link

Fix 417 #425

Closed timronan closed 4 years ago

timronan commented 4 years ago

Thank your for contributing to PH5

Before submitting a PR, please review the pull request guidelines: https://github.com/PIC-IRIS/PH5/blob/master/CONTRIBUTING.md#submitting-a-pull-request Pull request to replace PR #423 one my commits did not make it into this issues.

What does this PR do?

Address issue 417 by skipping traces that do not contain data. fixes #417

Relevant Issues?

Issue 417

Checklist

timronan commented 4 years ago

This is ready for review.

dsentinel commented 4 years ago

@timronan Would it make sense to add a test?

timronan commented 4 years ago

Yeah we should probably add a test to this. I will work on it soon.

timronan commented 4 years ago

Test was added to prove this PR works. Refer to commit a4442a11b1b32441b04983213531c5ddb7635603.

timronan commented 4 years ago

Are there any additional steps that should be taken fro this PR to be accepted?

damhuonglan commented 4 years ago

Are there any additional steps that should be taken fro this PR to be accepted?

Look good to me. I already approved it. Just wait for @dsentinel .

damhuonglan commented 4 years ago

@timronan According to our software meeting today, you can merge this PR with only one approval. So it's ok for you to merge this now.

timronan commented 4 years ago

Hi @damhuonglan,

I have a few questions to make sure that I am doing the pull request process correctly. Can you verify that I am supposed to merge my pull request. Am I supposed to do a squash and merge? I can't find anything in the https://github.com/PIC-IRIS/PH5/blob/master/CONTRIBUTING.md#submitting-a-pull-request page that outlines how I am supposed to merge my branch.

Currently my ability to merge is blocked because I have outstanding 'Changes Requested.' I think that I resolved all of these requested changes but will continue to make code changes if necessary. Please let me know how to proceed.

damhuonglan commented 4 years ago

@timronan I already marked all change requests as resolved for you. However, recently the travis-ci check got stuck for any PRs that have new commits that prevent merging. At first I thought that is for a specific PR, but now it happens to yours too. I will talk with Lloyd about this. He has been too busy recently and may be slow on this. Please be patient.

timronan commented 4 years ago

@damhuonglan I think you are correct and that Travis CI is stuck for this PR. No problem on the timing, I know Lloyd is busy, let me know if there is anything I need to do.

damhuonglan commented 4 years ago

@timronan Travis CI is not stuck anymore. But for Lloyd's requested change, it doesn't go away after I marked at resolved. Please look at the part that he request to change, it isn't marked as Outdated, you should change something in that part and commit to make it Outdated. That will remove the requested change left that prevent the merging.

timronan commented 4 years ago

Thanks for approving this pull request. @damhuonglan and/or @dsentinel is this pull request ready to merge? Which merging pattern should be used to merge a PH5 pull request?