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

fix: be more graceful when MS tool moves files underneath us #314

Merged
merged 1 commit into from
May 20, 2024

Conversation

mikix
Copy link
Contributor

@mikix mikix commented May 20, 2024

It moves temporary files into their final place, and that might happen as we are trying to get the file size - so handle that exception.

Checklist

  • Consider if documentation (like in docs/) needs to be updated
  • Consider if tests should be added

It moves temporary files into their final place, and that might happen
as we are trying to get the file size - so handle that exception.
Copy link

@Dtphelan1 Dtphelan1 left a comment

Choose a reason for hiding this comment

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

Only one thing I double checked and wanted to be sure about, but otherwise I think this looks good to me. Once the Error thrown is either confirmed as correct here or fixed, then you're good to go

def _get_file_size_safe(path: str) -> int:
try:
return os.path.getsize(path)
except FileNotFoundError:
Copy link

@Dtphelan1 Dtphelan1 May 20, 2024

Choose a reason for hiding this comment

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

(comment being written from an outsider's perspective, so forgive stupid questions plz)
Is this the right Error? The 3.12 docs I was looking at for os.path.getsize suggest it would throw OSError

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah yeah as we mentioned on slack, it's a builtin subclass of OSError.

@mikix mikix merged commit ceabc53 into main May 20, 2024
3 checks passed
@mikix mikix deleted the mikix/safer-file-size branch May 20, 2024 17:33
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.

2 participants