Skip to content

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

def __init__(self, filename):
self.filename = filename
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.

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