Skip to content

Conversation

@ojacques
Copy link

This change sorts the files when doing the import.

@CLAassistant
Copy link

CLAassistant commented Aug 17, 2017

CLA assistant check
All committers have signed the CLA.

@coveralls
Copy link

coveralls commented Aug 17, 2017

Coverage Status

Coverage remained the same at 93.326% when pulling c166ee7 on ojacques:sorted_paths into fbc7b04 on jmathai:master.

Copy link
Owner

@jmathai jmathai left a comment

Choose a reason for hiding this comment

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

Thanks for this PR.

I'd like to add some tests for this and make sure that it doesn't have any performance impact for large imports before merging.

If we merge this then we should do the same for the update command. https://github.com/jmathai/elodie/blob/master/elodie.py#L240

@ojacques
Copy link
Author

Thanks @jmathai - let me look at tests and the update function.
(and thanks for Elodie (my sister's name) !)

@jmathai
Copy link
Owner

jmathai commented Aug 22, 2017

Great. If you have questions then let me know. I could always open a PR with tests against your branch.

Copy link
Owner

@jmathai jmathai left a comment

Choose a reason for hiding this comment

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

Thanks for the PR. Sorry it's taken a while to look at. If you can do this then I'll merge else I'll create a separate pull request in the future and merge.

files.add(path)

for current_file in files:
for current_file in sorted(files):
Copy link
Owner

Choose a reason for hiding this comment

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

Can merge this if we switch to using the sort method on files. Sorted makes a copy which takes up additional memory while .sort() is in-place and seems okay in this case.

@jmathai jmathai force-pushed the master branch 4 times, most recently from 3276ccf to 12c17c9 Compare July 12, 2019 08:45
@jmathai jmathai force-pushed the master branch 2 times, most recently from bbdb460 to ec11497 Compare October 29, 2025 05:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants