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 exact_match to list_folders #507

Merged
merged 6 commits into from
Feb 25, 2022
Merged

Added exact_match to list_folders #507

merged 6 commits into from
Feb 25, 2022

Conversation

srivarra
Copy link
Contributor

If you haven't already, please read through our contributing guidelines before opening your PR

What is the purpose of this PR?

This PR closes #360. It adds an exact_match function parameter to the list_folders function. In addition it adds specific tests for
the case where exact_match is set to True.

How did you implement your changes

  1. The same case from list_files was used.
  2. Instances of using .sort() within assert statements were replaced with sorted(), as .sort() returns None.

Remaining issues

There could be more instances of using asserts with .sort()
instead of sorted().

For example:

a = [6,5,4,3]

assert a.sort() == [3,4,5,6]

Would not pass as a.sort() returns None.

However:

a = [6,5,4,3]

assert sorted(a) == [3,4,5,6]

Would pass as sorted() returns the sorted iterable.

Copy link
Member

@ngreenwald ngreenwald left a comment

Choose a reason for hiding this comment

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

The build is failing due to the linter being unhappy with your code. When you run the tests locally, make sure to add the --pycodestyle flag to pytest so that you'll see the same errors as github. You can google pep8 style guide to read up best practices

image

Copy link
Contributor

@alex-l-kong alex-l-kong left a comment

Choose a reason for hiding this comment

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

Just a couple testing comments.

ark/utils/io_utils_test.py Outdated Show resolved Hide resolved
ark/utils/io_utils_test.py Outdated Show resolved Hide resolved
Copy link
Contributor

@alex-l-kong alex-l-kong left a comment

Choose a reason for hiding this comment

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

Looks good, liking the new test_list_folders structure with separate sections for exact_match = False and exact_match = True.

Copy link
Member

@ngreenwald ngreenwald left a comment

Choose a reason for hiding this comment

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

Syntax looks nice, see comments below

ark/utils/io_utils_test.py Outdated Show resolved Hide resolved
ark/utils/io_utils_test.py Outdated Show resolved Hide resolved
@ngreenwald ngreenwald merged commit f927fb4 into master Feb 25, 2022
@ngreenwald ngreenwald deleted the substr-matching branch February 25, 2022 22:22
@srivarra srivarra added the bug Something isn't working label Jun 15, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Need better substring matching for list_folders
3 participants