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

Abstract reusable components #51

Merged
merged 70 commits into from
Feb 15, 2024
Merged

Conversation

ELC
Copy link
Collaborator

@ELC ELC commented Jan 28, 2024

@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:

  • Add Python 3.12 to CICD matrix
  • Add poetry.lock to git for reproducible environments in CICD

Overall:

  • Remove Linkset
  • Add FrozenDict as alternative of LinkSet
  • Sort imports and __all__

Docs:

  • Add linked tabs for code snippets
  • Add examples for all the formats (URLFor, HAL and SIREN) in basic and advanced docs
  • Re-write the index to avoid repetetive information
  • Adapt the extending chapter to the latest changes

HAL:

  • Use FrozenDict instead of LinkSet
  • Fix naming HalHyperModel to HALHyperModel
  • Divide Hypermodel and Response in individual files

Siren:

  • Separate components into individual files

Examples:

  • Minor changes in parameter order
  • Add response_model_exclude_unset=True to HAL and Siren examples

@ELC ELC marked this pull request as ready for review February 4, 2024 01:08
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] <[email protected]>
@jtc42
Copy link
Owner

jtc42 commented Feb 10, 2024

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.

@ELC
Copy link
Collaborator Author

ELC commented Feb 11, 2024

@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

@jtc42
Copy link
Owner

jtc42 commented Feb 12, 2024

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?

@ELC
Copy link
Collaborator Author

ELC commented Feb 12, 2024

@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

@jtc42
Copy link
Owner

jtc42 commented Feb 13, 2024

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.

@ELC
Copy link
Collaborator Author

ELC commented Feb 13, 2024

@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.

@jtc42
Copy link
Owner

jtc42 commented Feb 13, 2024

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 (fastapi-hypermedia, fastapi-hypermodel2, something like that?)

Sorry there's probably somewhere better to take this discussion than on a PR, but we're here now 😂

@ELC
Copy link
Collaborator Author

ELC commented Feb 13, 2024

@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 :)

@jtc42
Copy link
Owner

jtc42 commented Feb 15, 2024

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 jtc42 merged commit 7ff2b53 into jtc42:main Feb 15, 2024
6 checks passed
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