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

Unintuitive API #117

Open
1 of 4 tasks
sfermigier opened this issue Dec 27, 2023 · 3 comments
Open
1 of 4 tasks

Unintuitive API #117

sfermigier opened this issue Dec 27, 2023 · 3 comments
Labels
bug Something isn't working

Comments

@sfermigier
Copy link
Contributor

sfermigier commented Dec 27, 2023

Description

I have recently been bitten by the two following issues, in sequence:

  • With repository.get(): in Python, get is one of the most commonly used methods, and it's easy to assume that it means "return something if found, or None (or some other default value) otherwise". In advanced-alchemy's case, it raises an exception when nothing is found.

  • With repository.get_one_or_none(), it's easy to assume that it has a similar signature than get. But no, one has to provide the primary key by name (e.g. get(id) v.s get_one_or_none(id=id).). This can also lead to confusion.

In other words, there is a lack of consistency between:

  • repository.get() and dict.get()
  • repository.get() and repository.get_one_or_none()

It's probably too late to change the API now, but I suggest anyway:

  • renaming get to get_one and introducing a get method similar to the current version of get that return None instead of raising a exception.
  • Renaming the current get_* methods using a different verb ("fetch"?, "retrieve"? ...)

URL to code causing the issue

No response

MCVE

No response

Steps to reproduce

No response

Screenshots

No response

Logs

No response

Jolt Project Version

0.6.1.

Platform

  • Linux
  • Mac
  • Windows
  • Other (Please specify in the description above)

Funding

  • If you would like to see an issue prioritized, make a pledge towards it!
  • We receive the pledge once the issue is completed & verified
Fund with Polar
@sfermigier sfermigier added the bug Something isn't working label Dec 27, 2023
@cofin
Copy link
Member

cofin commented Dec 27, 2023

@sfermigier thanks for reporting this, and I do see what you mean.

Since we've not hit a stable API yet, I think we still potentially change this by adding some deprecated flags around the original methods.

FWIW, the original intent was to mirror the Django ORM behavior for get. However, now that we have quite a few additional methods maybe we should revisit this.

Let me think about how we might be able to accomplish this.

@cofin
Copy link
Member

cofin commented Jan 3, 2024

@sfermigier @jolt-org/maintainers

I've been thinking about this a bit. What do you think about renaming the get method to get_by_id? This should solve the confusion with the get method similarities.

@sfermigier
Copy link
Contributor Author

I have since realized that the SQLAlchemy Session object also provides a get method ("Return an instance based on the given primary key identifier, or None if not found.).

So I think get is a pretty reasonable name, as long as it returns None if not found.

get_by_id would still leave the expectation that it behaves like get, so I'm not sure about your proposition.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

2 participants