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

Add --exclude parameter to exclude directory to import #340 #342

Merged
merged 7 commits into from Oct 28, 2019

Conversation

@gti96
Copy link
Contributor

gti96 commented Sep 27, 2019

For #340.
I have tested this and it seems to be working for me. Also support multiple directories to exclude. Not sure if I have done anything that's not good practice though. Please check.

@CLAassistant

This comment has been minimized.

Copy link

CLAassistant commented Sep 27, 2019

CLA assistant check
All committers have signed the CLA.

Copy link
Owner

jmathai left a comment

Thanks for this PR. Appreciate the simplicity. In addition to the comments are you able to add some tests for _import and get_all_files? If you have questions about adding tests let me know.

elodie.py Outdated
@@ -110,6 +112,9 @@ def _import(destination, source, file, album_from_folder, trash, allow_duplicate

files = set()
paths = set(paths)
excludeDir = set()
if exclude:
excludeDir.update(exclude)

This comment has been minimized.

Copy link
@jmathai

jmathai Sep 28, 2019

Owner

Instead of excluding a directory can we take in a regular expression to be matched against the file path? I think then we won't need to use a set() since it looks that only let you use in.

This comment has been minimized.

Copy link
@gti96

gti96 Sep 28, 2019

Author Contributor

I use set() to accept optional multiple exclusions. Is there another way to do it? Please remember I am a newbie. 😄 And pointing me to how to add tests would be appreciated.

This comment has been minimized.

Copy link
@jmathai

jmathai Sep 30, 2019

Owner

Got it. I didn't realize multiple=True in the click decorator does this. Looks good.

for dirname, dirnames, filenames in os.walk(path):
dirnames[:] = [d for d in dirnames if d not in excludeDir]

This comment has been minimized.

Copy link
@jmathai

jmathai Sep 28, 2019

Owner

Instead of matching directories I think matching any part of the file path makes it more flexible.

This comment has been minimized.

Copy link
@gti96

gti96 Sep 28, 2019

Author Contributor

I will need some hints on how to do this then. Any suggestion on what function/method to use, so I can look it up? Which part on what file you think I should have this in?

This comment has been minimized.

Copy link
@gti96

gti96 Sep 28, 2019

Author Contributor

Perhaps something like below?

if excludeDir not in os.path.join(dirname, filename)
    yield os.path.join(dirname, filename)

This comment has been minimized.

Copy link
@jmathai

jmathai Sep 30, 2019

Owner

What's dirnames used for? I'm not seeing it referenced in the rest of the function.

I think logic similar to what you proposed should work but it seems like we should do it on the file names and not the directory names. Instead of this could we do something like the comment below?

Copy link
Owner

jmathai left a comment

Thanks for your feedback. Left a couple comments. It might be easier for me to submit a pull request on your branch with a test once you have the code changes made. Then once you merge the pull request I can merge yours back in here.

elodie.py Outdated
@@ -110,6 +112,9 @@ def _import(destination, source, file, album_from_folder, trash, allow_duplicate

files = set()
paths = set(paths)
excludeDir = set()
if exclude:
excludeDir.update(exclude)

This comment has been minimized.

Copy link
@jmathai

jmathai Sep 30, 2019

Owner

Got it. I didn't realize multiple=True in the click decorator does this. Looks good.

for dirname, dirnames, filenames in os.walk(path):
dirnames[:] = [d for d in dirnames if d not in excludeDir]

This comment has been minimized.

Copy link
@jmathai

jmathai Sep 30, 2019

Owner

What's dirnames used for? I'm not seeing it referenced in the rest of the function.

I think logic similar to what you proposed should work but it seems like we should do it on the file names and not the directory names. Instead of this could we do something like the comment below?

for dirname, dirnames, filenames in os.walk(path):
dirnames[:] = [d for d in dirnames if d not in excludeDir]
for filename in filenames:
# If file extension is in `extensions` then append to the list
if os.path.splitext(filename)[1][1:].lower() in extensions:

This comment has been minimized.

Copy link
@jmathai

jmathai Sep 30, 2019

Owner

Something like this is what I was thinking. I did not try to run this so there maybe errors :).

This takes the set of strings passed into --exclude and compiles them into a list of regular expression objects. Then for each file path (including directory) it will skip any which match against the expression.

For your original bug request this should work using --exclude=".*@eaDir.*", I think.

# Create a list of compiled regular expressions to match against the file path
exclude_regexes = [re.compile(regex) for regex in excludeDir]
filename_path = os.path.join(dirname, filename)
if (
        os.path.splitext(filename)[1][1:].lower() in extensions and
        not any(regex.match(filename_path) for regex in exclude_regexes)
    ):
    yield filename_path
@jmathai jmathai changed the title adding --exclude parameter to exclude directory to import Add --exclude parameter to exclude directory to import #340 Oct 1, 2019
@jmathai

This comment has been minimized.

Copy link
Owner

jmathai commented Oct 1, 2019

I submitted a PR by adding a few commits to your branch. PTAL to confirm that it works for your original goal in #340 to exclude @eaDir folders. If you can merge it to your master branch I'll merge this PR.

@gti96

This comment has been minimized.

Copy link
Contributor Author

gti96 commented Oct 2, 2019

I am trying to find out how I can test your commits from your PR. Other than copy and paste your codes to mine, could you please show me how to do it easily?

@jmathai

This comment has been minimized.

Copy link
Owner

jmathai commented Oct 2, 2019

Here are instructions and you'll need to pass in --exclude-regex="@eaDir".

I think this should also be available via the configuration file so you don't always have to pass it in through the command line.

# create a new branch named jmathai-gti96-master to stage the changes
git checkout -b jmathai-gti96-master master
# pull changes from my repo/branch into the branch you created/switched to above
git pull https://github.com/jmathai/elodie.git gti96-master

FYI - these instructions are also provided next to the "merge" button on the PR I opened under "command line instructions".

Also --- generally speaking I recommend making changes to your clone in a separate branch than master. This will make it easier for you to merge changes from upstream (my repo) back into yours.

The nice thing about git is that you can restore any branch to any commit very easily.

@gti96

This comment has been minimized.

Copy link
Contributor Author

gti96 commented Oct 10, 2019

This is working great and it accepts optional multiple regex in case it's needed. Great work. Thanks.

@jmathai

This comment has been minimized.

Copy link
Owner

jmathai commented Oct 11, 2019

@gti96 I submitted another pull request which adds support for specifying exclusions in a configuration file so you don't always have to pass it in to the import command. Also added documentation. Once you merge it and tests pass I'll merge it here.

Added ability to set exclusions in the configuration file and documentation
@jmathai jmathai merged commit 75e6590 into jmathai:master Oct 28, 2019
4 checks passed
4 checks passed
Scrutinizer Analysis: 7 new issues, 14 updated code elements – Tests: passed
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls First build on master at 90.819%
Details
license/cla Contributor License Agreement is signed.
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.