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

Cache lib url check #43

Merged
merged 1 commit into from Jul 26, 2021
Merged

Cache lib url check #43

merged 1 commit into from Jul 26, 2021

Conversation

jannschu
Copy link
Contributor

@jannschu jannschu commented Jul 25, 2021

This pull request adds a simple cache to the mermaid_lib property. The reason is that for projects with many pages the url check with url_exists(...) is done redundantly. It decreased the build time for my setup significantly.

Python 3.8 has a cached_property decorator in functools but I guess that is probably not the minimal Python version you want to support. I could use functools.lru_cache if you prefer, though.

edit: Fixes #44.

@fralau
Copy link
Owner

fralau commented Jul 25, 2021

  1. I find it very interesting (and a good sign) that this project is getting optimization-oriented PRs (this is the second; see: Do not parse HTML pages if we know there are no mermaid charts #41 ).

  2. For a question of uniformity, could I ask you to open an issue that describes the problem you are solving, and set the Linked issues in this PR?

  3. This might be a good use case for @cached_property, but the considerations of backward compatibility are valid. IMHO, using an explicit attribute is good coding practice. I tend to declare it as Null, and set it, rather than testing for its existence. But about tastes and colors... 🙂

@jannschu
Copy link
Contributor Author

jannschu commented Jul 25, 2021

Ok, I added an issue and linked it.

Regarding point 3: A disadvantage of defining an attribute for the cached value in, say, __init__ with None, is that you can not distinguish the cached value from the initial value, None in this case, in the event they are equal for some reason. Another reason to only use the attribute in the property code is that in this way it is clear that it is not intended to be used in the rest of the class (matter of taste). But I hope I understood you correctly there, feel free to edit :-)

edit: forgot a "not"

@fralau
Copy link
Owner

fralau commented Jul 26, 2021

@jannschu Argument taken about point 3. Which is why I concede it is a matter of taste (or rather common sense) 🙂

@fralau fralau merged commit 149a4b1 into fralau:master Jul 26, 2021
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.

Cache check whether library URL exists slow down build
2 participants