Skip to content

Correct cd history when going to package not on current PWD#28

Open
SuperJappie08 wants to merge 4 commits into
colcon:masterfrom
SuperJappie08:cd-back
Open

Correct cd history when going to package not on current PWD#28
SuperJappie08 wants to merge 4 commits into
colcon:masterfrom
SuperJappie08:cd-back

Conversation

@SuperJappie08

Copy link
Copy Markdown

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
Copy Markdown
Author

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

Comment thread function/colcon_cd.sh
echo "Could not find package '$1' from the current working" \
"directory" 1>&2
fi
OLDPWD="$_colcon_cd_old_pwd"

Copy link
Copy Markdown
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
Copy Markdown
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?

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

@nuclearsandwich are the comments enough to address your feedback here?

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

Copy link
Copy Markdown
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
Copy Markdown

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

cottsay commented Oct 8, 2025

Copy link
Copy Markdown
Member

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants