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

Skip download in CDN mode #22

Merged

Conversation

maiksprenger
Copy link
Contributor

@maiksprenger maiksprenger commented Mar 25, 2025

Motivation:

This is a follow-up to #20

#21 added a CDN feature, which means the added script tag will point to the CDN version of mermaid.js. Thank you again for adding that!

But django-mermaid still tries to download a copy to the package's static directory in AppConfig.ready. Because this isn't necessary in CDN mode and causes us a PermissionError during deployment, I'm proposing we only download in non-CDN mode.

All Submissions:

  • Have you followed the guidelines in our Contributing document?
  • Have you checked to ensure there aren't other open Pull Requests for the same update/change?

Changes to Core Features:

  • Have you added an explanation of what your changes do and why you'd like us to include them?
  • Have you updated the documentation related to the changes you have made?
  • Have you written new tests for your core changes, as applicable?
  • Have you successfully run tests with your changes locally?

This is a follow-up to
pysnippet#20

pysnippet#21 added a CDN feature,
which means the added script tag will point to the CDN version of
mermaid.js. But it still tries to download a copy to the package's
static directory in AppConfig.ready.

Because this isn't necessary in CDN mode and causes us a PermissionError
during deployment, I'm proposing we only download in non-CDN mode.
I think it's a better fit there, because the other tests all concern the
template tag itself.

Please note that the test fails for me locally.
@maiksprenger maiksprenger changed the title Skip download in cdn mode Skip download in CDN mode Mar 25, 2025
@maiksprenger
Copy link
Contributor Author

Thank you for considering, @ArtyomVancyan !

When this is merged, the package will be used improving documentation at a company with nearly a thousand engineers. I think if it's ready for that, it's time to give it a 0.1.0 release, if not even 1.0.0. What do you think? :)

@ArtyomVancyan
Copy link
Member

@maiksprenger, thanks for the PR. I totally agree that it shouldn't try to download the assets when it is in CDN mode. Liked the changes in the tests section (separation was a good idea). Also, don't think that 1.0.0 will be appropriate after 0.0.9, so just bumped it to 0.1.0, although 0.0.10 would also be fine as the patch is not big :)

@ArtyomVancyan ArtyomVancyan merged commit 88741ef into pysnippet:master Mar 25, 2025
22 checks passed
@maiksprenger
Copy link
Contributor Author

Amazing, thank you for merge and release!

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