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

DM-35487: Tweak cardinal directions for All Sky camera #87

Merged
merged 4 commits into from
Jul 20, 2022

Conversation

ugyballoons
Copy link
Collaborator

No description provided.

Copy link
Contributor

@jonathansick jonathansick left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good. I've just got a recommendation to cache the asset versioning function.

@@ -334,6 +349,10 @@ def month_names() -> List[str]:
return [date(2000, m, 1).strftime("%B") for m in list(range(1, 13))]


def get_static_resource_timestamp() -> int:
return int(os.path.getmtime("src/rubintv/static/stylesheets/main.css"))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This'll work, but I think there's a limitation here. I don't think git tracks filesystem metadata like modification date so this timestamp effectively gets set when the Docker image gets built (my expectation, at least). So this'll successfully burst caches, but won't let a browser used the cached style sheet or other asset files between rubintv builds — which isn't much of a performance loss.

The way I'd kind of expect to implement this is by doing some sort of hash calculation on the content during app start up and cache that in memory (like in the aiohttp app context or via functools.cache). But again, no real harm in what you've got.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, a hash generator that ran on startup across all the static files would be much better. I think that would take me a while to write though.

It's frustrating that aiohttp offers just this functionality but I can't find anything in the docs about how to use the corresponding url_for() function in the templates.

@@ -334,6 +349,10 @@ def month_names() -> List[str]:
return [date(2000, m, 1).strftime("%B") for m in list(range(1, 13))]


def get_static_resource_timestamp() -> int:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps try functools.cache to reduce the filesystem hits? The result shouldn't change, so it's easy to use here.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's not something I've come across before. How does it work?
Is this a case of just adding a @cache decorator before the def? (After pulling functools in)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right. It's a function decorator, so it'll look like:

Suggested change
def get_static_resource_timestamp() -> int:
import functools
@functools.cache
def get_static_resource_timestamp() -> int:

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's great, thanks!

@ugyballoons ugyballoons merged commit 0e54870 into master Jul 20, 2022
@ugyballoons ugyballoons deleted the tickets/DM-35487 branch July 20, 2022 14:26
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