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

Added directory and file utils to replace tinydir #218

Closed
wants to merge 3 commits into from

Conversation

ahcorde
Copy link
Contributor

@ahcorde ahcorde commented Mar 27, 2020

This PR is part of the effort to reduce the number of dependencies in ROS 2. Tinydir is only used in RCL in one place to find a folder that match with a string.

I have created a new file directory_and_file_reader.[hc] maybe I can include this code in filesystem.[hc].

Signed-off-by: ahcorde [email protected]

@ahcorde ahcorde added the enhancement New feature or request label Mar 27, 2020
@ahcorde ahcorde self-assigned this Mar 27, 2020
@mjcarroll
Copy link
Member

Just a note that this shouldn't have a major documentation impact as tinydir is currently built/installed via a vendor package.

Copy link
Member

@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.

Some comments on the documentation.

I can't tell if this is completely covered by the current filesystem tests. Does this need additional test coverage?

include/rcutils/directory_and_file_reader.h Outdated Show resolved Hide resolved
include/rcutils/directory_and_file_reader.h Show resolved Hide resolved
@dirk-thomas
Copy link
Member

Does this need additional test coverage?

The new API certainly needs test coverage.

@ahcorde
Copy link
Contributor Author

ahcorde commented Mar 30, 2020

Added test coverage 98a64ec

@dirk-thomas dirk-thomas removed their request for review March 30, 2020 22:22
@ahcorde
Copy link
Contributor Author

ahcorde commented Mar 31, 2020

According with the comment of @mikaelarguedas in rcl ros2/rcl#605 (comment) this code probably makes no sense.

@dirk-thomas should we close both PRs?

@ahcorde
Copy link
Contributor Author

ahcorde commented Mar 31, 2020

  • Linux Build Status
  • Linux-aarch64 Build Status
  • macOS Build Status
  • Windows Build Status

Copy link
Member

@wjwwood wjwwood left a comment

Choose a reason for hiding this comment

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

lgtm, though I just skimmed it. Some comments/nitpicks.

/// is the file a directory ?
int is_dir;
// information about the file
#ifndef _WIN32
Copy link
Member

Choose a reason for hiding this comment

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

nitpick: these preprocessor directives are usually not indented.

int error = closedir(dir->dir);
// The closedir() function returns 0 on success
if (error) {
#else
Copy link
Member

Choose a reason for hiding this comment

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

nitpick: this indent is weird too

#else
// If the function succeeds, the return value is nonzero.
if (!FindClose(dir->dir)) {
#endif
Copy link
Member

Choose a reason for hiding this comment

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

GitHub is failing to render this, maybe that's the source of your uncrustify issue too?

if (lstat(
#else
if (_tstat(
#endif
Copy link
Member

Choose a reason for hiding this comment

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

Actually, this stanza is likely the issue... I'd recommend using a #define for this function name if that's all that really is different.

@dirk-thomas
Copy link
Member

According with the comment of @mikaelarguedas in rcl ros2/rcl#605 (comment) this code probably makes no sense.
should we close both PRs?

If the API is not used anymore, yes, I think it should just be removed.

@ahcorde ahcorde closed this Apr 1, 2020
@ahcorde ahcorde deleted the ahcorde/remove_tinydir branch April 10, 2020 19:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants