Skip to content

Conversation

@SuperJappie08
Copy link

Corrects the cd jump back behavior to work as expected #27.

@SuperJappie08 SuperJappie08 marked this pull request as ready for review July 19, 2024 14:33
@SuperJappie08
Copy link
Author

@cottsay, Friendly ping to prevent this PR from being open for many years.

echo "Could not find package '$1' from the current working" \
"directory" 1>&2
fi
OLDPWD="$_colcon_cd_old_pwd"
Copy link
Contributor

Choose a reason for hiding this comment

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

My understanding is that this change is here so that if no package is found and thus no change in directory is finalized this restores the shell context back as if when running cd normally you tried to cd into a non-existent directory. It might be worth going through and annotating for future contributors when OLDPWD and the internal _colcon_cd_old_pwd variables are set, updated and unset to make the maintenance easier on future contributors.

Copy link
Author

Choose a reason for hiding this comment

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

I added some comments. Are they clear enough?

Signed-off-by: SuperJappie08 <36795178+SuperJappie08@users.noreply.github.com>
@SuperJappie08
Copy link
Author

@cottsay, The current test failure is in an unrelated part of the code.
Should I also try to fix it in this PR?

@codecov-commenter
Copy link

Welcome to Codecov 🎉

Once you merge this PR into your default branch, you're all set! Codecov will compare coverage reports and display results in all future pull requests.

Thanks for integrating Codecov - We've got you covered ☂️

@cottsay
Copy link
Member

cottsay commented Oct 8, 2025

The current test failure is in an unrelated part of the code.
Should I also try to fix it in this PR?

I fixed it in master and merged the fix back into this PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

4 participants