-
Notifications
You must be signed in to change notification settings - Fork 6
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
Abstract reusable components #51
Conversation
Bumps [ruff](https://github.com/astral-sh/ruff) from 0.1.14 to 0.2.1. - [Release notes](https://github.com/astral-sh/ruff/releases) - [Changelog](https://github.com/astral-sh/ruff/blob/main/CHANGELOG.md) - [Commits](astral-sh/ruff@v0.1.14...v0.2.1) --- updated-dependencies: - dependency-name: ruff dependency-type: direct:development update-type: version-update:semver-minor ... Signed-off-by: dependabot[bot] <support@github.com>
May have merged out of order, could you please take a look at the conflicts when you get time? Also not particularly keen to commit the poetry.lock file as the published library does not include pinned dependencies, and dependencies may resolve differently depending on the Python version. The reproducibility is really important for applications, but for a library like this I think it's better we know how the dependencies will resolve on each version in the CICD pipeline. |
@jtc42 Conflicts have been resolved, moreover, the latest commit also includes the changes requested by dependabot I believe the lock works a little bit different than you may think. The wheel, and thus the package as a library will have as dependencies what has been specified in the pyproject.toml in the dependencies table, it ignores the lock altogether. The purpose of committing the .lock file is just so that the developers of the libraries will have exactly the same environment, this is specially important in the CICD, I faced an issue with the CICD using a different version of Ruff (2.x vs 1.x), these types of issues will never happen to users of the libraries though as it is a dev dependency. Regarding resolving based on the Python version, that is accurate but only relevant for final distribution not for CICD. If we would like to test if dependencies are resolving so that it works for all versions regardless of how dependencies may be resolved, the solution is not to avoid committing the lock but to either (1) use stricter constrains in the dependencies so that it works always or (2) always re-lock dependencies with update so that it is tested that works with the latest compatible versions of transient dependencies. Not commuting the .lock to simulate how the environment of the user may be is too uncertain, there are too many scenarios where installations will fail that are unavoidable and untestable. For example if the users pins Pydantic to 1.x they will not be able to use this library, regardless of the specific version that is in the pyproject.toml or in the lock file. Happy to discuss this further :) I believe the best approach is to keep dependencies as loosen as possible, commit the lock and keep it updated to prevent breaking with recent releases from transient dependencies. For this though we would need to test with the oldest versions possible as well, to make sure nothing breaks, not only with the latest. This is non-trivial as detailed in python-poetry/poetry#3527 |
I'm aware how the lock file works. See https://python-poetry.org/docs/basic-usage/#as-a-library-developer Give the extremely small number of developers on this library, reproducible developer environments are far less important to me than knowing about potentially breaking dependency changes as they occur. What that has reminded me is that I really do need to set up a scheduled CI test run. I'll sort this out soon. It seems like there's pros and cons to each, but given the rationale in the link above, and the reasons given, I'd like to continue omitting the lock file for now. This can be trivially changed down the line as needed, but definitely should not be rolled into a PR relating to something else entirely. Is this otherwise ready to be merged? |
@jtc42 Sounds good, not commiting the .lock but having a schedule run is another alternative. We still need to figure out a similar approach that would work for minimal versions rather than latest ones. We may need to play around with Tox Generative environments. I also agree this should be further discussed in a seperate branch/PR This is ready to merge now |
I probably should have asked prior to now, but is the context here that you're now using this library in some production environment where stricter QC is required? If so, that's worth me knowing as until now this has only been used by a handful of people, and as far as I'm aware not in any critical environment. I ask because until now the priority has been keeping maintenance simple given my limited time and the libraries limited usage, but depending on the context that tradeoff may need to be re-assessed. |
@jtc42 not at the moment, I been very enthusiastic about Hypermedia recently and this was the most recent project that included support for both Pydanic and FastAPI. I am not planning on using this in a production environment now, for that I would need to check which is the impact regarding performance. I do not worry though about dependencies since the constraints seems fine. My work on this library was on my own free time and as a side project. I do have a technical blog and plan to write about Hypermedia and this library would be great to show some concrete examples. I do have plans to continue adding new features and formats in the future and would really appreciate it if we do an ownership transfer. |
That's really cool, thanks for the extra info. Would love to see a blog post around this kind of stuff, sounds super interesting. Regarding ownership transfer, I would prefer to either add you as a maintainer here, or perhaps suggest using your fork as the in-development version. If you'd prefer the latter, I'm more than happy to link to your fork from the README explaining the situation, though I would maybe suggest appending something to the library name for publishing ( Sorry there's probably somewhere better to take this discussion than on a PR, but we're here now 😂 |
@jtc42 being maintainer here would work and would keep things simple for anyone reading later on. We can continue with the feature branch strategy if you want (although I am biased towards trunk-based development), and keep discussing topics and ideas in Issues and PRs. I lean towards not overcomplicating things so issues + PRs should suffice (I'm saying using Github "Project" board may be overkill). As a long term vision, I see this library being used for a large crowd, potentially extending to other frameworks like Flask or Django, but my preference would be to first support as many Hypermedia formats as possible while keeping the code clean, tested and documented. Happy to continue the discussion somewhere else as well. If you agree, I'd like to merge this and have it published to PyPI as a new major so the latest features are available for everyone :) |
That all sounds good. I'd much prefer to keep working with feature branches if that's ok, but yeah everything else sounds good. I'l add you as a maintainer now. |
@jtc42
This is a stacked PR over #49, merge that PR first before reviewing this.
This PR has quality of life and structure changes, no new features were added:
CICD:
Overall:
__all__
Docs:
HAL:
HalHyperModel
toHALHyperModel
Siren:
Examples:
response_model_exclude_unset=True
to HAL and Siren examples