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

🎨 Why is list_source called this way and not .sources or similar? #1863

Open
falexwolf opened this issue Aug 28, 2024 · 11 comments
Open

🎨 Why is list_source called this way and not .sources or similar? #1863

falexwolf opened this issue Aug 28, 2024 · 11 comments
Assignees

Comments

@falexwolf
Copy link
Member

It looks like it returns a QuerySet and not a list.

image
@falexwolf
Copy link
Member Author

falexwolf commented Aug 28, 2024

So, the above could simply make use of the pattern that we'd encourage, rather than requiring users to learn a new pattern:

bt.CellType.sources.get(currently_used=True)

@falexwolf falexwolf changed the title Why is list_source called this way and not .sources or similar? 🎨 Why is list_source called this way and not .sources or similar? Aug 28, 2024
@sunnyosun
Copy link
Member

It needs to be .sources()

@sunnyosun
Copy link
Member

laminlabs/bionty#116

@sunnyosun
Copy link
Member

sunnyosun commented Sep 17, 2024

@falexwolf can you confirm you want this? laminlabs/bionty#116

I don't think .sources is possible because it must be a class attribute.

@falexwolf
Copy link
Member Author

Hm. I don't get the reason why this isn't a ManyToMany accessor. Why isn't it?

We should have a naming scheme for such cases that behave like a ManyToMany but actually aren't.

How about query_sources()? But I gotta admit that any naming scheme seems odd if .sources can't be a many to many.

@sunnyosun
Copy link
Member

Because it's not a relationship! Look at the schema, the relationship is a FK for each record called .source.

.sources() is a method in the base class.

@sunnyosun
Copy link
Member

It's at the class level!

@falexwolf
Copy link
Member Author

Why don't we recommend the following?

bt.Source.filter(created_cell_types__isnull=False).all()

This is 100% equivalent to getting all the users who, e.g., created artifacts. We should have a single way for users to query this inverse relationships. The easiest would be to rely on existing Django idioms for that matter, I'd say. 🤔

Unfortunately, we might just have removed the easy way to query for all Source records. This Slack thread.

@sunnyosun
Copy link
Member

The inverse relationship is removed.

@sunnyosun
Copy link
Member

bt.Source.filter(created_cell_types__isnull=False).all()
This is to query for which sources are linked to the existing records.

.list_source() is getting available sources independent of what ontology records are in the registry!

Even if bt.CellType is empty, bt.CellType.list_sources() will return all sources!

@falexwolf
Copy link
Member Author

is getting available sources independent of what ontology records are in the registry

But that's even easier to achieve with

bt.Source.filter(entity="CellType").all()

I don't understand why we don't clearly document that query and rather introduce something non-idiomatic.

This is already a long list for which it's hard to develop intuition for:
image

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

No branches or pull requests

2 participants