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

Add getDirectoryContents' which ignores "." and ".." #36

Merged
merged 3 commits into from Oct 3, 2015

Conversation

Projects
None yet
2 participants
@guibou
Contributor

guibou commented Sep 25, 2015

getDirectoryContents returns the "." and ".." special directories. However in most cases user do not care about theses values and must manually filter them. This pull request introduces the getDirectoryContents' variant which handle this filtering. I find this really convenient when working with directory listing. Other languages uses the same trick (such as python, with os.listdir(),

Discussion: should we update removeContentsRecursive to use getDirectoryContents' instead of the explicit manual filtering of "." and ".." ?

@Rufflewind

This comment has been minimized.

Show comment
Hide comment
@Rufflewind

Rufflewind Sep 25, 2015

Member

Perhaps instead of using a prime (which may be confused for other things), we could call it getDirectoryContentsA to mirror the -A flag for ls. I was originally thinking of getDirectoryContentsPlain but the name is already too long.

should we update removeContentsRecursive to use getDirectoryContents' instead of the explicit manual filtering of "." and ".." ?

I don't see any reason not to :P

Thanks for the pull request!

Member

Rufflewind commented Sep 25, 2015

Perhaps instead of using a prime (which may be confused for other things), we could call it getDirectoryContentsA to mirror the -A flag for ls. I was originally thinking of getDirectoryContentsPlain but the name is already too long.

should we update removeContentsRecursive to use getDirectoryContents' instead of the explicit manual filtering of "." and ".." ?

I don't see any reason not to :P

Thanks for the pull request!

@guibou

This comment has been minimized.

Show comment
Hide comment
@guibou

guibou Sep 26, 2015

Contributor

Thank you for your reply. I will amend the pull request to change the name to getDirectoryContentsA, you are right that the prime is confusing, I initially saw it as an alternate version, but apparently it is commonly used for strict version of a function.

I also edited removeContentsRecursive to use the new getDirectoryContentsA. My last question is about the unit tests. For now there is 29 tests which introduce a use of getDirectoryContents (and a special handling of . and ..).

For the sake of simplifing the unit test, should we edit them to use getDirectoryContentsA in most cases ?

Contributor

guibou commented Sep 26, 2015

Thank you for your reply. I will amend the pull request to change the name to getDirectoryContentsA, you are right that the prime is confusing, I initially saw it as an alternate version, but apparently it is commonly used for strict version of a function.

I also edited removeContentsRecursive to use the new getDirectoryContentsA. My last question is about the unit tests. For now there is 29 tests which introduce a use of getDirectoryContents (and a special handling of . and ..).

For the sake of simplifing the unit test, should we edit them to use getDirectoryContentsA in most cases ?

@guibou

This comment has been minimized.

Show comment
Hide comment
@guibou

guibou Sep 26, 2015

Contributor

I amended the initial commit to change the name to getDirectoryContentsA, updated removeContentsRecursive to use it, and updated tests to use getDirectoryContentsA when it simplifies the tests.

Contributor

guibou commented Sep 26, 2015

I amended the initial commit to change the name to getDirectoryContentsA, updated removeContentsRecursive to use it, and updated tests to use getDirectoryContentsA when it simplifies the tests.

@Rufflewind

This comment has been minimized.

Show comment
Hide comment
@Rufflewind

Rufflewind Sep 26, 2015

Member

updated tests to use getDirectoryContentsA when it simplifies the tests.

While that's fine for the other tests, for tests/GetDirContents001.hs it's best to leave both getDirectoryContents and getDirectoryContentsA in to ensure that getDirectoryContents is covered by at least one test (to prevent a future change from accidentally removing . and .. from getDirectoryContents).

Member

Rufflewind commented Sep 26, 2015

updated tests to use getDirectoryContentsA when it simplifies the tests.

While that's fine for the other tests, for tests/GetDirContents001.hs it's best to leave both getDirectoryContents and getDirectoryContentsA in to ensure that getDirectoryContents is covered by at least one test (to prevent a future change from accidentally removing . and .. from getDirectoryContents).

@Rufflewind Rufflewind merged commit 0e7e7a3 into haskell:master Oct 3, 2015

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@guibou

This comment has been minimized.

Show comment
Hide comment
@guibou

guibou Oct 3, 2015

Contributor

Thank you for merging it. Sorry I did not answer your complain about the unit test not covering . and .. for getDirectoryContents (I was busy this week...).

I'll soon do another pull request to fix that (an some other stuff if time is available).

Contributor

guibou commented Oct 3, 2015

Thank you for merging it. Sorry I did not answer your complain about the unit test not covering . and .. for getDirectoryContents (I was busy this week...).

I'll soon do another pull request to fix that (an some other stuff if time is available).

@Rufflewind

This comment has been minimized.

Show comment
Hide comment
@Rufflewind

Rufflewind Oct 3, 2015

Member

I'll soon do another pull request to fix that

Don't worry about that – it's all taken care of. Thanks for your contribution!

Member

Rufflewind commented Oct 3, 2015

I'll soon do another pull request to fix that

Don't worry about that – it's all taken care of. Thanks for your contribution!

@Rufflewind

This comment has been minimized.

Show comment
Hide comment
@Rufflewind

Rufflewind Oct 12, 2015

Member

Just a heads up in case you happen to be using HEAD: I've renamed getDirectoryContentsA to listDirectory.

Member

Rufflewind commented Oct 12, 2015

Just a heads up in case you happen to be using HEAD: I've renamed getDirectoryContentsA to listDirectory.

bgamari pushed a commit to bgamari/directory that referenced this pull request Jul 29, 2016

Merge pull request haskell#36 from thomie/searchPath
Remove double quotes around searchPath elements on Windows
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment