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

Filter out empty elements when creating command strings #609

Merged
merged 3 commits into from
Apr 5, 2021

Conversation

christophebedard
Copy link
Member

Follow-up to #608, which didn't fix every occurrence.

@christophebedard christophebedard requested a review from a team as a code owner April 4, 2021 20:00
@christophebedard christophebedard requested review from lucashan and prajakta-gokhale and removed request for a team April 4, 2021 20:00
@codecov
Copy link

codecov bot commented Apr 4, 2021

Codecov Report

Merging #609 (d8fe2fe) into master (bd36a6b) will increase coverage by 0.55%.
The diff coverage is 37.50%.

❗ Current head d8fe2fe differs from pull request most recent head 5dde110. Consider uploading reports for the commit 5dde110 to get more accurate results
Impacted file tree graph

@@            Coverage Diff             @@
##           master     #609      +/-   ##
==========================================
+ Coverage   38.28%   38.83%   +0.55%     
==========================================
  Files           2        2              
  Lines         222      224       +2     
  Branches       42       43       +1     
==========================================
+ Hits           85       87       +2     
  Misses        137      137              
Impacted Files Coverage Δ
src/action-ros-ci.ts 29.74% <37.50%> (+0.72%) ⬆️

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 bd36a6b...5dde110. Read the comment docs.

@christophebedard christophebedard force-pushed the christophebedard/fix-extra-spaces branch from 9234e55 to 5eaeba0 Compare April 4, 2021 20:09
Signed-off-by: Christophe Bedard <bedard.christophe@gmail.com>
@christophebedard christophebedard force-pushed the christophebedard/fix-extra-spaces branch from 5eaeba0 to ecee94e Compare April 4, 2021 20:41
@christophebedard
Copy link
Member Author

Looking at the tests now, it all looks good.

@@ -467,7 +467,9 @@ done`;
`--packages-up-to ${packageNames}`,
`${extra_options.join(" ")}`,
extraCmakeArgs !== "" ? `--cmake-args ${extraCmakeArgs}` : "",
].join(" ");
]
.filter((e) => e.length > 0)
Copy link
Contributor

Choose a reason for hiding this comment

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

these are all fine, but now that we are doing it 4 times, i wonder if a utility function would be warranted, e.g.

function filterNonEmptyJoin(values: List) {
  return values.filter((v) => v.length > 0).join(" ")
}

Copy link
Member Author

Choose a reason for hiding this comment

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

sure I can do that!

I can even add a test 😁

Copy link
Member Author

Choose a reason for hiding this comment

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

utility function

done in 1ed8912 with test in 5dde110

Signed-off-by: Christophe Bedard <bedard.christophe@gmail.com>
Signed-off-by: Christophe Bedard <bedard.christophe@gmail.com>
Copy link
Contributor

@emersonknapp emersonknapp left a comment

Choose a reason for hiding this comment

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

nice, thanks

@emersonknapp emersonknapp enabled auto-merge (squash) April 5, 2021 19:32
@emersonknapp emersonknapp merged commit 086849c into master Apr 5, 2021
@emersonknapp emersonknapp deleted the christophebedard/fix-extra-spaces branch April 5, 2021 20:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants