pepkit / pepdbagent

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

Dev #24

Closed khoroshevskyi closed 1 year ago

khoroshevskyi commented 1 year ago

added new get_annotations functions

nleroy917 commented 1 year ago

Are tests failing because of database connection issues?

khoroshevskyi commented 1 year ago

Are tests failing because of database connection issues?

Yes

nleroy917 commented 1 year ago

Some small things with questions. Biggest being splitting up the upload_project function into two functions. Also unit tests

khoroshevskyi commented 1 year ago

@nleroy917 @nsheff I have reviewed all comment's and fixed it. In my opinion there is one think we should add: Documentation.

nleroy917 commented 1 year ago

I am testing this with pephub and it looks like you might be missing a line in the requirements file for coloredlogs

nleroy917 commented 1 year ago

Also, since you've changed the class name to PepAgent, I think the __init__.py file needs to reflect this:

Otherwise, I get: ImportError: cannot import name 'PepAgent' from 'pepagent'

khoroshevskyi commented 1 year ago

Also, since you've changed the class name to PepAgent, I think the __init__.py file needs to reflect this:

Otherwise, I get: ImportError: cannot import name 'PepAgent' from 'pepagent'

Good catch! Additionally we are thinking about changing pepagent name to pepdbagent and PepDbAgent class. What do you thing about that naming @nleroy917 ? I think this name can be to long but it reproduces actual meaning of this package @nsheff

nleroy917 commented 1 year ago

Additionally we are thinking about changing pepagent name to pepdbagent and PepDbAgent class

Yeah that sounds good to me. Or maybe Connection? Similar to the relational database drivers. E.g. psycopg2.connect -> Connection.

I'm impartial, however.

khoroshevskyi commented 1 year ago

I like it!

khoroshevskyi commented 1 year ago

@nleroy917 @nsheff I think, it's ready.

nleroy917 commented 1 year ago

I think, it's ready.

If you dont mind, before a merge I might test drive locally with pephub.