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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

New option cache_safe_extras #19

Closed
wants to merge 8 commits into from
Closed

New option cache_safe_extras #19

wants to merge 8 commits into from

Conversation

kamilkrzyskow
Copy link
Contributor

Hi, 馃憢
I'm new to mkDocs and came across an issue with cached extra css/js files.
I read here mkdocs/mkdocs#1979 that it's an ongoing issue since years, and is supposedly being solved in the next 1.5.0 version https://github.com/mkdocs/mkdocs/milestone/15

I didn't want to wait, but also didn't want to dive deep into plugin/mkDocs development, then I found this plugin that both accesses the files and edits the names to .min.ext, so I knew I could simply adjust it slightly and add the hashes to the file names.

In the process of adding the new option I've:

  • "cleaned up" the project simplifying some parts of the code,
  • run isort, black -l100, pylint, and mypy. Manually, as I didn't want to setup automated hooks and tinker with configuring them 馃槄,
  • changed order of functions in the files to keep the flow of reading similar to the flow of execution,
  • added some tests before and after the changes, so it works on something more than a fluke (I hope 馃槅).

I've also bumped the version of all dependencies to latest and Python >=3.7 to align with the mkDocs 1.4.1. The added f-strings and type hints break support for older <3.6 Python versions, but I don't really see this as an issue.

If I done too many changes, and you'd like only some part of it, I can cooperate 馃

@wilhelmer
Copy link
Collaborator

Thanks for contributing! Indeed, these are a lot of changes, I'm not sure what to do with this. I could approve everything in good faith, as you are probably a better developer than me. But it feels a bit overwhelming.

Also, this PR addresses at least two topics at once: a) the new cache_safe_extras option (which is great, love this), b) code cleanup/refactoring/adding tests/etc.

It would be great if you could you split this in multiple PRs, so we could theoretically add the caching option without changing the entire code base. I know it's some extra work, but I think it's better for everyone involved in this project.

@kamilkrzyskow
Copy link
Contributor Author

@wilhelmer
Thanks for the quick response, I'll split the PR into multiple, but the question is into how many? Should I just make each of the commits a separate PR?馃
I can definitely see that new tests (with old structure) and new option will likely be approved.
Should I then add further PRs like update dependencies, code simplification, code style changes, documentation or is it unnecessary?

I started with the code cleanup, because I was in the process of figuring out how things work here, so I could add the new option with more ease, however I understand the idea of "don't fix what isn't broken", so I expected a decline for the PR merge after I saw how much I changed.馃槄

I'll send the 2 PRs later today or tomorrow.

@wilhelmer
Copy link
Collaborator

Two PRs should be enough IMHO. One for the new option (including the test for that option + docs), one for everything else.

Thanks 馃憤

@kamilkrzyskow
Copy link
Contributor Author

Continued in #20

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.

None yet

2 participants