Skip to content

Simplify removing leading and trailing separators#712

Merged
cottsay merged 2 commits intomasterfrom
cottsay/simplify-prefix-util
Jan 16, 2026
Merged

Simplify removing leading and trailing separators#712
cottsay merged 2 commits intomasterfrom
cottsay/simplify-prefix-util

Conversation

@cottsay
Copy link
Copy Markdown
Member

@cottsay cottsay commented Oct 15, 2025

This change is adapted from ament/ament_package#152, authored by @robwoolley.

Previously, we checked if the leading or trailing character was a colon and then used a wildcard to remove it.

It is simpler to just remove a leading or trailing colon. This has the added benefit of only using shell built-in functions.

Requires tests that are added in #713.

Previously, we checked if the leading or trailing character was a colon
and then used a wildcard to remove it.

It is simpler to just remove a leading or trailing colon. This has the
added benefit of only using shell built-in functions.

Co-authored-by: Scott K Logan <logans@cottsay.net>
@cottsay cottsay self-assigned this Oct 15, 2025
@cottsay cottsay added the enhancement New feature or request label Oct 15, 2025
@codecov
Copy link
Copy Markdown

codecov Bot commented Oct 15, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 86.98%. Comparing base (8557dee) to head (8e69d8d).
⚠️ Report is 1 commits behind head on master.

Additional details and impacted files
@@           Coverage Diff           @@
##           master     #712   +/-   ##
=======================================
  Coverage   86.98%   86.98%           
=======================================
  Files          69       69           
  Lines        4088     4088           
  Branches      706      706           
=======================================
  Hits         3556     3556           
  Misses        421      421           
  Partials      111      111           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@cottsay cottsay changed the base branch from master to cottsay/shell-tests October 21, 2025 18:51
@cottsay cottsay closed this Oct 21, 2025
@cottsay cottsay reopened this Oct 21, 2025
@cottsay cottsay marked this pull request as ready for review October 21, 2025 18:57
@cottsay cottsay added this to the 0.20.2 milestone Oct 21, 2025
@cottsay cottsay force-pushed the cottsay/shell-tests branch 2 times, most recently from cba4e28 to e31f37f Compare October 21, 2025 23:46
@cottsay
Copy link
Copy Markdown
Member Author

cottsay commented Oct 22, 2025

So this is sort of interesting, because I uncovered some issues in #713 that are solved by this PR, but not because of the lack of -c support for head or tail that originally motivated the change. It appears that on macOS, I have echo -n foo working as expected in zsh, but when I switch to sh, the /bin/echo executable that's used doesn't accept -n anymore so the conditional never fires.

It doesn't actually fail, it just doesn't remove the leading/trailing separators like it should.

Given that there's actually buggy behavior on a supported platform, I'm a little more motivated to get this pushed through and get #713 merged with some actual tests here.

Copy link
Copy Markdown

@knmcguire knmcguire left a comment

Choose a reason for hiding this comment

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

I've tested out these solutions directly in a bash terminal and I can confirm this working.

Just a comment that it won't remove any double colons before and after the environment variable path string, nor does it remove any double colons in between. But I've learned that this is not really an edge case that has happened before, if at least nobody has reported this at least...

@cottsay
Copy link
Copy Markdown
Member Author

cottsay commented Jan 16, 2026

Just a comment that it won't remove any double colons before and after the environment variable path string, nor does it remove any double colons in between. But I've learned that this is not really an edge case that has happened before, if at least nobody has reported this at least...

Agreed on all points. The primary purpose for this is to ensure we aren't making things worse.

@cottsay cottsay changed the base branch from cottsay/shell-tests to master January 16, 2026 22:11
@cottsay cottsay merged commit 35e65f6 into master Jan 16, 2026
46 checks passed
@cottsay cottsay deleted the cottsay/simplify-prefix-util branch January 16, 2026 22:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request

Development

Successfully merging this pull request may close these issues.

3 participants