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

clarify CATKIN_IGNORE may be a file, directory, or (dangling) symlink #256

Merged
merged 3 commits into from
May 4, 2020

Conversation

rotu
Copy link
Contributor

@rotu rotu commented Apr 29, 2020

Mention that CATKIN_IGNORE may be any type of file and its contents are ignored

Mention that CATKIN_IGNORE may be any type of file
change "are ignored" to "don't matter" to avoid implying that that's what "IGNORE" is referring to in "CATKIN_IGNORE"
Copy link
Member

@dirk-thomas dirk-thomas left a comment

Choose a reason for hiding this comment

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

Beside the inline question please update the title of the PR since it will become the commit message.

rep-0128.rst Outdated Show resolved Hide resolved
@rotu rotu changed the title Update rep-0128.rst CATKIN_IGNORE may be any type of file Apr 29, 2020
rotu added a commit to rotu/colcon-core that referenced this pull request Apr 29, 2020
Copy link
Member

@dirk-thomas dirk-thomas left a comment

Choose a reason for hiding this comment

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

Widening the possible types for the marker sounds reasonable to me. I don't expect any breakage being introduced due to this change (like users have directories or (dangling) symlinks with this name but not intending to ignore the path).

@dirk-thomas
Copy link
Member

@ros-infrastructure/ros_team Please review and approve this proposed change.

Copy link
Contributor

@mjcarroll mjcarroll left a comment

Choose a reason for hiding this comment

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

I don't see anything wrong with this, but I am curious how you use it in practice? Is there a benefit to making the marker files a directory or symlink?

rotu added a commit to rotu/colcon-core that referenced this pull request Apr 29, 2020
@mjcarroll
Copy link
Contributor

Is there a benefit to making the marker files a directory or symlink?

Answered my own question: ros-infrastructure/catkin_pkg#286 (comment)

Copy link
Contributor

@jacobperron jacobperron left a comment

Choose a reason for hiding this comment

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

LGTM

@dirk-thomas dirk-thomas changed the title CATKIN_IGNORE may be any type of file clarify CATKIN_IGNORE may be a file, direcotry, or (dangling) symlink May 4, 2020
@dirk-thomas dirk-thomas changed the title clarify CATKIN_IGNORE may be a file, direcotry, or (dangling) symlink clarify CATKIN_IGNORE may be a file, directory, or (dangling) symlink May 4, 2020
@dirk-thomas
Copy link
Member

3 👍 and 5 days sounds like enough to me. Merging...

@dirk-thomas dirk-thomas merged commit c1e8136 into ros-infrastructure:master May 4, 2020
dirk-thomas pushed a commit to colcon/colcon-core that referenced this pull request May 4, 2020
* Allow ignore_marker to be a dangling symlink

As per ros-infrastructure/rep#256

* os.path.lexists instead of (nonexistent) pathlib.path.lexists
prajakta-gokhale pushed a commit to colcon/colcon-bundle that referenced this pull request May 13, 2020
* Respect ignore marker even if dangling symlink

As per ros-infrastructure/rep#256

* os.path.lexists instead of (nonexistent) pathlib.path.lexists

Co-authored-by: Prajakta Gokhale <prajaktg@amazon.com>
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

4 participants