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

Auto-execute all files from a glob or a dir #39

Merged
merged 5 commits into from
Oct 25, 2019

Conversation

samuelhamard
Copy link
Contributor

@codecov-io
Copy link

codecov-io commented Sep 6, 2019

Codecov Report

Merging #39 into master will increase coverage by <.01%.
The diff coverage is 95.23%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #39      +/-   ##
==========================================
+ Coverage   93.72%   93.73%   +<.01%     
==========================================
  Files          13       13              
  Lines         685      702      +17     
  Branches      124      128       +4     
==========================================
+ Hits          642      658      +16     
  Misses         20       20              
- Partials       23       24       +1
Impacted Files Coverage Δ
django_north/management/commands/migrate.py 92.92% <95.23%> (+0.24%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 329b05f...b13451c. Read the comment docs.

self._load_schema_fullpath_file(path)
else:
for file_path in sorted(glob.glob(path)):
if os.path.isfile(file_path) and file_path.endswith('.sql'):
Copy link
Contributor

Choose a reason for hiding this comment

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

But what if the glob points to a dir, though ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't think it's a expected behavior to load multiple files and dird over a glob. And we don't want a recursive search.

I don't know use case to include a glob of dirs, and I don't think it was desired.

But I can do it if you think it's better ?

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't know. I'd find it logical if it did, but I really don't know if it's ever going to be used. For me it's more along the lines of "it it simplifies the code, let's do it".

Copy link
Contributor

@ewjoachim ewjoachim left a comment

Choose a reason for hiding this comment

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

This is good,

I'd like to see if it's possible to split a little more the logic and the I/Os., and burry the I/Os a little less.

I see 3 I/O operations:

  • glob.glob (that matches with the disks contents)
  • Seeing of a path is a dict
  • Reading the file and running it.

Would it be possible to do the work in 3 phases:

  • Use glob to get all the paths
  • Expand dirs on all the paths
  • run script on all resulting paths

That would be 3 sequential loops instead of nesting loops and ifs (flat is better than nested). If it's not very clear, get in touch. Anyway, I'm not opposed to merging as-is as well.

Use only the glob to find files on `NORTH_BEFORE_SCHEMA_FILES`
and `NORTH_AFTER_SCHEMA_FILES`
@samuelhamard samuelhamard merged commit 523d9d4 into master Oct 25, 2019
@samuelhamard samuelhamard deleted the Auto-Execute_all_files_from_a_glob branch October 25, 2019 09:44
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.

Auto-execute all files from a folder
3 participants