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

Introduce the File tool and tweak stub binary implementation #1871

Merged
merged 3 commits into from
Jun 10, 2024

Conversation

rmartin16
Copy link
Member

@rmartin16 rmartin16 commented Jun 9, 2024

Changes

  • Introduce the File tool
    • Encapsulates complex file operations such as archive unpacking
    • Incorporates the (now removed) Download tool's functionality
  • Some updates to the stub binary error handling

Notes

PR Checklist:

  • All new features have been tested
  • All new features have been documented
  • I have read the CONTRIBUTING.md file
  • I will abide by the code of conduct

super().__init__(f"Unable to unpack support package {filename!r}")
super().__init__(f"Unable to unpack support package '{filename}'.")
Copy link
Member Author

Choose a reason for hiding this comment

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

If you just call repr() here and filename is a Path, it looks like this:

Unable to unpack support package PosixPath("/asdf")

so, this strategy gets what we actually want.

Unable to unpack support package '/asdf'

Copy link
Member

Choose a reason for hiding this comment

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

Torn between whether we should use the explicit ', or {str(filename)!r} - that should ensure that any inline quotes are correctly escaped (I think...)

Copy link
Member Author

Choose a reason for hiding this comment

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

You mean like these differences?

>>> file_path=Path("/pa't'h/to/file")
>>> 
>>> print(f"File is {file_path}")
File is /pa't'h/to/file
>>> 
>>> print(f"File is {str(file_path)!r}")
File is "/pa't'h/to/file"
>>> 
>>> print(f"File is '{file_path}'")
File is '/pa't'h/to/file'

Manually calling str() does seem to handle that edge case best.

Copy link
Member

Choose a reason for hiding this comment

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

That's what I had in mind, yes.

@rmartin16 rmartin16 marked this pull request as ready for review June 9, 2024 18:54
@rmartin16 rmartin16 marked this pull request as draft June 9, 2024 21:11
@rmartin16
Copy link
Member Author

ive nerdsniped myself in to implementing a File tool....

@rmartin16 rmartin16 closed this Jun 9, 2024
@rmartin16 rmartin16 reopened this Jun 9, 2024
@rmartin16 rmartin16 changed the title Tweaks to out-of-template stub binary implementation Introduce the File tool and tweak stub binary implementation Jun 9, 2024
- Encapsulates complex file operations such as archive unpacking
- Incorporates the (now removed) Download tool's functionality
@rmartin16
Copy link
Member Author

The updates for the File tool were relatively extensive but also pretty banal. Reviewing the changes for each commit will help discern what I changed for the stub binary versus the File tool.

@rmartin16 rmartin16 marked this pull request as ready for review June 10, 2024 00:39
Copy link
Member

@freakboy3742 freakboy3742 left a comment

Choose a reason for hiding this comment

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

A large PR, but largely straightforward, with some nice cleanups along the way. One minor suggestion about some possibly overenthusiastic modularity; weak preference would be to remove it, but if you feel particularly strongly to the contrary, I'm not going to fight you too hard over it.

},
)

def _unpack_archive_kwargs(self, archive_path: str | os.PathLike) -> dict[str, str]:
Copy link
Member

Choose a reason for hiding this comment

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

Possibly a little over-enthusiastic refactoring here; function calls aren't free, and in practice, this method can't be overwritten by a subclass. Given it's wrapping a single if clause that won't be needed in a couple of Python releases, I'm not sure the extra method is worth it.

Copy link
Member Author

Choose a reason for hiding this comment

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

That makes sense. I also tweaked it a bit so (in theory, anyway) pyupgrade should have an easier time trimming it down later on.

@rmartin16
Copy link
Member Author

A new problem from this is a corrupted build directory if the stub can't be installed.

Wasn't there work being done to ensure the bundle was deleted if the CreateCommand goes bad?

Copy link
Member

@freakboy3742 freakboy3742 left a comment

Choose a reason for hiding this comment

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

👍 LGTM.

@freakboy3742 freakboy3742 merged commit b1d86a0 into beeware:main Jun 10, 2024
52 checks passed
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