Skip to content

Fix issue with moving temporary files#178

Merged
mhidas merged 3 commits intomasterfrom
mphemming_temp_file_fix
Jan 16, 2023
Merged

Fix issue with moving temporary files#178
mhidas merged 3 commits intomasterfrom
mphemming_temp_file_fix

Conversation

@mhidas
Copy link
Copy Markdown
Contributor

@mhidas mhidas commented Jan 11, 2023

Thanks @mphemming for identifying and fixing this issue.
(commits cherry-picked from #176)

So the issue is that the temp file is still open in Spyder, meaning that shutils cannot move the file at the end of the main aggregator function.

I edited line 214 to include 'fd' as output:
_, temp_outfile = tempfile.mkstemp(suffix='.nc', dir=output_dir) to fd, temp_outfile = tempfile.mkstemp(suffix='.nc', dir=output_dir).

I can then close the object adding the following code to line 355 prior to using 'shutil.move':

os.close(fd)

This now works without an error for both 'aggregated_timeseries.py' and 'velocity_aggregated_timeseries.py'.
Used the same code as aggregated products
@codecov
Copy link
Copy Markdown

codecov Bot commented Jan 11, 2023

Codecov Report

Base: 85.54% // Head: 85.58% // Increases project coverage by +0.03% 🎉

Coverage data is based on head (a2ca613) compared to base (2b1f81e).
Patch coverage: 100.00% of modified lines in pull request are covered.

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #178      +/-   ##
==========================================
+ Coverage   85.54%   85.58%   +0.03%     
==========================================
  Files          10       10              
  Lines        1128     1131       +3     
  Branches      156      153       -3     
==========================================
+ Hits          965      968       +3     
  Misses        144      144              
  Partials       19       19              
Impacted Files Coverage Δ
...tools/timeseries_products/aggregated_timeseries.py 95.20% <100.00%> (+0.02%) ⬆️
...eseries_products/velocity_aggregated_timeseries.py 91.91% <100.00%> (+0.05%) ⬆️
.../timeseries_products/velocity_hourly_timeseries.py 95.23% <100.00%> (+0.02%) ⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@mphemming
Copy link
Copy Markdown

@mhidas happy for you to proceed with this one. I'll start a new branch for future pull requests ..

@mhidas mhidas marked this pull request as ready for review January 16, 2023 00:39
@mhidas mhidas requested a review from leonardolaiolo January 16, 2023 00:57
@mhidas
Copy link
Copy Markdown
Contributor Author

mhidas commented Jan 16, 2023

@leonardolaiolo This is a quick & easy one to review.
The changes are actually from @mphemming but I can't merge it without an approving review, and GitHub doesn't let me request a review from him...
(Let me know if you'd like more context)

@leonardolaiolo
Copy link
Copy Markdown
Contributor

Done @mhidas - I'll leave it to you to merge then.

@mhidas mhidas merged commit e032a76 into master Jan 16, 2023
@mhidas mhidas deleted the mphemming_temp_file_fix branch January 16, 2023 03:28
@mhidas mhidas restored the mphemming_temp_file_fix branch January 16, 2023 03:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants