pepkit / pepdbagent

Database for storing sample metadata
BSD 2-Clause "Simplified" License
2 stars 1 forks source link

Restructuring #65

Closed khoroshevskyi closed 1 year ago

khoroshevskyi commented 1 year ago

Everything that was mention in previous PR is here pepdbagent divided into 5 Modules:

Let me know if you have any questions

Few things to add before merging:

nleroy917 commented 1 year ago

So, are we going to close the other PR?

khoroshevskyi commented 1 year ago

@nleroy917 @nsheff I've fixed all your comments. I think, now it's on the stage, that we can start implementing this version of pepdbagent into pephub

nsheff commented 1 year ago

Hey @Khoroshevskyi this API is really nice, I really like it. This is way more clear now!

I went through the tutorial and edited it a bit. There are few things that still confused me, and I wasn't sure how to edit it. Can you rephrase these points:

nsheff commented 1 year ago

Here are some ideas for word changes, what do you think?

nleroy917 commented 1 year ago

+1 for word changes @nsheff suggested.

nsheff commented 1 year ago

"# submit a project" -> "# create a project"

nsheff commented 1 year ago

Why is there only a _by_rp function for annotation.get_by_rp? Do you need functions create_by_rp and update_by_rp, et. and put them on project, annotation, namespace modules?

nsheff commented 1 year ago

Instead of "NamespaceReturnModel" it doesn't make sense to put the word "Return" in this object, because it's not always going to be "Returned" it could be used for lots of other things.

What is this really? Can you name it appropriately?

NamespaceList? Similar for Annotation? AnnotationList object maybe?

khoroshevskyi commented 1 year ago

Why is there only a _by_rp function for annotation.get_by_rp?

I don't really understand this question

Do you need functions create_by_rp and update_by_rp, et. and put them on project, annotation, namespace modules?

If we need I will add it, but I am not sure if we are using it. https://en.wikipedia.org/wiki/You_aren%27t_gonna_need_it

khoroshevskyi commented 1 year ago

Instead of "NamespaceReturnModel" it doesn't make sense to put the word "Return" in this object, because it's not always going to be "Returned" it could be used for lots of other things.

What is this really? Can you name it appropriately?

NamespaceList? Similar for Annotation? AnnotationList object maybe?

Yes, probably we should use different name for it. Your suggestions?? (p.s. In my experience all model naming should end on Model so in will be obvious that it's model)

nsheff commented 1 year ago

The pydantic docs don't suggest ending every model with Model: https://docs.pydantic.dev/usage/models/

I would suggest NamespaceList and AnnotationList ...

nsheff commented 1 year ago

Is this model an Annotation or Annotations ? Can you sync the model name and docstring one way or the other?

https://github.com/pepkit/pepdbagent/blob/9621f05e61e14b11af61e7800ca6c28a2f4288ae/pepdbagent/models.py#L9-L11

nsheff commented 1 year ago

Can you justify why you're prefixing double underscores here? I'm not familiar with that convention, can you explain why you're not just using the single underscore approach that I'm familiar with?

https://github.com/pepkit/pepdbagent/blob/9621f05e61e14b11af61e7800ca6c28a2f4288ae/pepdbagent/pepdbagent.py#L37-L43

nsheff commented 1 year ago

What's the advantage of attaching an object at _project (or __project) and then defining a @property called project that does nothing more than return the item at _project?

How is this superior to just sticking the object on .project in the first place?

https://github.com/pepkit/pepdbagent/blob/9621f05e61e14b11af61e7800ca6c28a2f4288ae/pepdbagent/pepdbagent.py#L39-L47

khoroshevskyi commented 1 year ago

1) two underscore means private object, it means that you can't change it outside of the object. (You can but in different way) 2) if I am using property, no one can change __project or __namespace variable outside pepdbagent

nleroy917 commented 1 year ago

The pydantic docs don't suggest ending every model with Model.

I was going to ask also about the naming of the models. For example, what's the difference between NamespaceResultModel and NamespaceReturnModel? I was getting them mixed up a bit.

khoroshevskyi commented 1 year ago

The pydantic docs don't suggest ending every model with Model.

I was going to ask also about the naming of the models. For example, what's the difference between NamespaceResultModel and NamespaceReturnModel? I was getting them mixed up a bit.

Yeah, again naming... I changed it already. so now you will have: NamespaceList: it returns model that has list of ~NamespaceRetrunModel~ Namespace ~NamespaceRetrunModel~ Namespace: is ONE result

AnnotationList: it returns model that has list of Annotation Annotation: is ONE annotation

nsheff commented 1 year ago

Can NamespaceResultModel just be Namespace then, for consistency with Annotation ?