Add management command to download htmx #428
Conversation
|
I feel like I've over-engineered some bits (like URL building), so feel free to strip out stuff you don't find useful :) |
| @@ -0,0 +1,77 @@ | |||
| from __future__ import annotations | |||
There was a problem hiding this comment.
How about we call the management command htmx and have a subparser for the download subcommand, allowing the possibility of more in the future?
There was a problem hiding this comment.
I have never considered Django management subcommands as I thought it's not idiomatic, but it doesn't sound like a bad idea at all!
| if TYPE_CHECKING: | ||
| from argparse import ArgumentParser |
There was a problem hiding this comment.
Only use if TYPE_CHECKING to avoid circular imports or other issues, no need for plain imports
| if TYPE_CHECKING: | |
| from argparse import ArgumentParser | |
| from argparse import ArgumentParser |
|
|
||
| class Command(BaseCommand): | ||
| help = "Download the given htmx version and the extensions." | ||
| requires_system_checks = [Tags.staticfiles] |
There was a problem hiding this comment.
Why did you do this? Were you trying to speed up the checks ?
| requires_system_checks = [Tags.staticfiles] |
There was a problem hiding this comment.
I was thinking that since we use the STATICFILES_DIRS, it makes sense to at least check if the finders are configured correctly. But I agree with the other comment of yours about not having this default, so it's not needed after all
| "-d", | ||
| type=Path, | ||
| default=None, | ||
| help="where to download HTMX to. default: the first STATICFILES_DIR", |
There was a problem hiding this comment.
I’d rather we didn’t have a default, every project has a different static layout.
There was a problem hiding this comment.
Should the script just accept a target directory, then?
| package_spec += f"@{options['version']}" | ||
|
|
||
| actual_version = options["version"] | ||
| for file in ["htmx.js", "ext/debug.js", "ext/event-header.js"]: |
There was a problem hiding this comment.
Extensions should be selected by the user on the command line, not hardcoded. The previous script hardcoded these extensions as they’re used in the example app. Add a --ext option.
| with urlopen(url) as response: | ||
| if not actual_version: | ||
| actual_version = ( | ||
| urlparse(response.geturl()).path.split("/")[1].split("@")[-1] |
There was a problem hiding this comment.
too much on one line, split up and name the intermediate parts in variables
|
I found the original issue requesting the feature, #87, and edited your message to feature it. |
| def handle(self, *args: str, **options: Any) -> None: | ||
| dest_dir = options["dest"] | ||
| if dest_dir is None: | ||
| if len(settings.STATICFILES_DIRS) == 0: |
There was a problem hiding this comment.
this setting isn’t guaranteed to exist, so we need to use getattr
|
Thanks for the initial review! I'll try to address this stuff tomorrow :) |
|
move to comment on the issue #87 |
for more information, see https://pre-commit.ci
| actual_version = options["version"] | ||
| files = ["htmx.js"] | ||
| if options["ext"] is not None: | ||
| files += [f"ext/{extension}" for extension in options["ext"]] |
There was a problem hiding this comment.
I just noticed that htmx v2 does not include extensions in the NPM package. I need to investigate where to source them from
There was a problem hiding this comment.
todo for me: think about how to define the new extensions to download, as just a name won't suffice...
|
Just now in #468, I refactored the example |
Closes https://fosstodon.org/@adamchainz/112086273489830574 ;)
Edit: fixes #87.