-
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
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
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! |
a93e2cc
to
2c91583
Compare
@alexpovel , sorry for the delay, busy days here... company announced reorg, so a lot happening! 😅 Added a section about the new function in the README.md, and tests to verify that WebHandler correctly fetches, caches and serves resume data from a URL while handling both successful and error scenarios. I checked ruff check/format, mypy, and pytest, results looks good, can you please take a look when possible? If good, can run the checks again? |
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, no worries!
It's looking good, thanks for adding tests and docs. I just have comments on some of the error handling (and tests thereof). Then it's good to go!
https://github.com/alexpovel/ancv/actions/runs/11845227596/job/33079082336?pr=241#step:7:1315 These errors are on me, sorry |
No problems, and don't worry, really appreciate the comments/time to review it, and I always do basic tests locally before to push, so they should be good..... in theory 😅 |
Thanks! Look good. There's still the |
…ound in the PR review
My bad, just pushed it, should be good now! =] |
Thank you for your contribution @kiraum ! I just merged... let's see what fails in CI now. I need to fix that flakiness... |
Thanks! My pleasure, learning a lot here! =] |
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