search

CCI-MOC / flocx-market

2 stars 9 forks source link

Create data model for an offer in marketplace #2

Closed DanNiESh closed 5 years ago

DanNiESh commented 5 years ago

This pecan app is connect to the MariaDB in a docker env. Instructions are listed in README.md. The offer data model (models.py) and related files are in flocx_market/db. An api to list all offers is in flocx_market/api/controllers/v1/offers.py.

larsks commented 5 years ago

Please squash all these commits together so that this PR contains only a single commit. Also consider updating the remaining commit message(s) as suggested in https://blog.oddbit.com/post/2019-06-14-git-etiquette-commit-messages/. Thank you!

tzumainn commented 5 years ago

Hi! Just a quick general comment: I think the flocx_market/api files should go in a separate commit. We don't want to overload a single commit with too much stuff; that makes it extremely difficult to review without getting overwhelmed. Ideally, this PR would be limited to defining the database models and allowing a user to load those models; that's a nice, easily testable chunk of work.

In regards to how to load those models, you should have a command similar to sudo esi-leap-dbsync create_schema in esi-leap. The relevant files in that repository include:

It's possible that it'll make sense to have a PR just for the common/ files first.

DanNiESh commented 5 years ago

Hi! Just a quick general comment: I think the flocx_market/api files should go in a separate commit. We don't want to overload a single commit with too much stuff; that makes it extremely difficult to review without getting overwhelmed. Ideally, this PR would be limited to defining the database models and allowing a user to load those models; that's a nice, easily testable chunk of work.

In regards to how to load those models, you should have a command similar to sudo esi-leap-dbsync create_schema in esi-leap. The relevant files in that repository include:

It's possible that it'll make sense to have a PR just for the common/ files first.

So do I need to make a new PR to define data model and load them?

tzumainn commented 5 years ago

I might suggest the following:

DanNiESh commented 5 years ago

I might suggest the following:

  • create a new PR that implements the common service code
  • keep this PR for the data model, and loading the data model, but remove the api code
  • create a new PR for the api code

Sure. Thank you for your suggestions!