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

Return path to decompressed file from decompress_file() #536

Merged
merged 1 commit into from
Aug 7, 2023
Merged

Return path to decompressed file from decompress_file() #536

merged 1 commit into from
Aug 7, 2023

Conversation

janosh
Copy link
Contributor

@janosh janosh commented Aug 7, 2023

@codecov
Copy link

codecov bot commented Aug 7, 2023

Codecov Report

❗ No coverage uploaded for pull request base (master@2b91561). Click here to learn what that means.
Patch has no changes to coverable lines.

Additional details and impacted files
@@            Coverage Diff            @@
##             master     #536   +/-   ##
=========================================
  Coverage          ?   76.49%           
=========================================
  Files             ?       27           
  Lines             ?     1523           
  Branches          ?      320           
=========================================
  Hits              ?     1165           
  Misses            ?      297           
  Partials          ?       61           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@shyuep shyuep merged commit 81e1211 into materialsvirtuallab:master Aug 7, 2023
6 checks passed
@Andrew-S-Rosen
Copy link
Contributor

@janosh: This seems like it was perhaps a breaking change. Could you take a look at https://github.com/Quantum-Accelerators/quacc/actions/runs/5790005449/job/15692210083?pr=660 and see if it seems related?

@Andrew-S-Rosen
Copy link
Contributor

Andrew-S-Rosen commented Aug 7, 2023

@janosh: Yeah, actually, seems like this introduce a bug. If the file extension isn't in ["BZ2", "GZ", or "Z"] then out_file is never referenced and an error is raised.

A better solution would be to have it return None in such a scenario, as this would yield the same behavior as the prior version of the function.

for f in files:
compress_file(os.path.join(parent, f), compression=compression)


def decompress_file(filepath):
def decompress_file(filepath) -> str:
Copy link
Contributor

Choose a reason for hiding this comment

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

@janosh: Change this to str | None

f_out.writelines(f_in)
os.remove(filepath)

return out_file
Copy link
Contributor

Choose a reason for hiding this comment

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

@janosh: move this return inside the if statement so that it only returns the string if it's decompressing a file. Otherwise, have it return None (as before).

@janosh janosh deleted the decompress_file-return-path branch August 7, 2023 22:23
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