-
Notifications
You must be signed in to change notification settings - Fork 10
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
feat(web): Add web server command to serve JSON resume from URL with periodic refresh #241
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for your PR! Looks nice! 🙂
A couple notes:
- devbox is totally optional, so if you can replicate it locally more or less, that's totally fine. CI uses devbox though, and that ultimately needs to pass. That would be the strongest argument in favor of devbox. That said, I've been thinking of ditching devbox too, because...
- ...I only used it for
poetry
, which sure enough broke down immediately when I tried to get ancv running locally again.uv
seems much more robust by itself! - does
[tool.uv] dev-dependencies
inpyproject.toml
work for you? It has all dev dependencies there. I'm new touv
so happy to change that if there's a better way
CI is currently failing, I think because of GitHub-specific API token problems... so never mind that for now.
Two general notes:
- you can add your new functionality to docs, e.g. the Selfhosting section of the README.
- a test will be needed I'm afraid! It should check for basic functionality. You you try and set the
destination
to a local server like https://docs.python.org/3/library/http.server.html , which is spun up for the test (similar to https://pkg.go.dev/net/http/httptest#NewServer if you're more familiar with Go). Checking the caching functionality would also be nice, you could setrefresh
to a second and play with that
ancv/web/server.py
Outdated
""" | ||
self.destination = destination | ||
self.refresh_interval = refresh_interval | ||
self.cache: Optional[str] = None |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This could be self.cache: Optional[ResumeSchema] = None
, see below for more!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I made the self.cache
strong typed str
, since the implementation is dealing with the rendered template, do you think that's better to change it to self.cache: Optional[ResumeSchema] = None
, and the functions to deal with that?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmmm, up to you!
In my mind the cache is for network I/O, and going from ResumeSchema
to a rendered str
is so cheap and fast it won't matter unless you're requesting your resume at crazy RPS. I just really hate str
as a type. It says and guarantees basically nothing. But if cache
as str
is a better fit, it's okay!
Thanks for the quick answer/comments, I will go over it during the week, a bit slow here, because it's herfstvakantie (autumn holidays), so a bit busy with family! 😅 I never tested I normally use something like this:
With that, I can compile the requirements and requiments only for dev env:
Check the final diff:
I will include the docs for the new functionality and tests, I'm trying to keep style as similar as I can, feel free to nitpick. |
6fe8ef9
to
8579672
Compare
Sounds great! Let's take all the time we like, this is about having fun!
I think I prefer
which is what we want. Those dependencies shouldn't occur anywhere but for local dev, and in CI. With optional deps, users could |
Just discovered: I refactored to use Lines 53 to 67 in e60f5f5
which is already outdated practice...: https://github.com/astral-sh/uv/releases/tag/0.4.27 aka PEP 735 will allow this to be expressed natively and standards-based, which is fantastic: [dependency-groups]
dev = [
"pytest >=8.1.1,<9"
] etc. But |
yeah, uv release is too fast, hard to follow 😅 I will give a try to that myself this week! |
This PR introduces a new
web
command forserve
allowing to serving an JSON resume from a URL with periodic refresh. Key features include:web
command in__main__.py
with options for URL, refresh interval, port, and host;WebHandler
class inserver.py
to handle fetching, rendering, and serving the resume;This feature enables users to serve their resume from external sources, making it easier to keep the displayed resume up-to-date without manual intervention.
Notes:
-- On my setup, I normally include
optional-dependencies
onpyproject
, with that I can useuv pip compile --extra dev > requirements-dev.txt
, and have a local pre-commit.curl https://ansiv.xpto.it
;poetry
touv
;Please, let me know if something doesn't look good, and/or if I missed something, and feel free to reject/close the PR if it's not something aligned with the project.
Thanks for the very nice project/repo/code, learning a lot with it! =D