-
Notifications
You must be signed in to change notification settings - Fork 36
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
django: Demo app created #63
Conversation
efe08d1
to
5b03c5b
Compare
This is ready for a review, I was able to get the sqlalchemy tests working again |
friendly ping |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: 0 of 15 files reviewed, 4 unresolved discussions (waiting on @jordanlewis, @rafiss, and @rohany)
python/django/cockroach_example/models.py, line 15 at r1 (raw file):
id = models.AutoField(primary_key=True) subtotal = models.DecimalField(max_digits=18, decimal_places=2) # TODO (rohany): is setting null the right thing here? The schema doesn't say what to do.
i think it would be fine to just make this cascade delete. that seems like the most basic thing to do here, and i think this demo app is just about showing off the most rudimentary ORM examples
python/django/cockroach_example/settings.py, line 88 at r1 (raw file):
DATABASES = { 'default': { # 'ENGINE' : 'django.db.backends.postgresql',
do we need the commented line?
python/django/cockroach_example/views.py, line 21 at r1 (raw file):
customers = list(Customers.objects.values()) else: customers = list(Customers.objects.filter(id=id).values())
question from my inexperience: is this is how you do a PK lookup in django?
python/django/cockroach_example/views.py, line 29 at r1 (raw file):
c = Customers(name=name) c.save() return HttpResponse(status=200)
doesn't really matter since it's just a demo app, but shouldn't the customer ID be returned back to the caller here? same for the other POST methods. this might not be worth changing if it would require other test infra changes
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: 0 of 15 files reviewed, 4 unresolved discussions (waiting on @jordanlewis and @rafiss)
python/django/cockroach_example/models.py, line 15 at r1 (raw file):
Previously, rafiss (Rafi Shamim) wrote…
i think it would be fine to just make this cascade delete. that seems like the most basic thing to do here, and i think this demo app is just about showing off the most rudimentary ORM examples
Done.
python/django/cockroach_example/settings.py, line 88 at r1 (raw file):
Previously, rafiss (Rafi Shamim) wrote…
do we need the commented line?
nop
python/django/cockroach_example/views.py, line 21 at r1 (raw file):
Previously, rafiss (Rafi Shamim) wrote…
question from my inexperience: is this is how you do a PK lookup in django?
this is how you do any generic field lookup. to use the default primary key, you'd do .filter(pk=whatever)
here.
python/django/cockroach_example/views.py, line 29 at r1 (raw file):
Previously, rafiss (Rafi Shamim) wrote…
doesn't really matter since it's just a demo app, but shouldn't the customer ID be returned back to the caller here? same for the other POST methods. this might not be worth changing if it would require other test infra changes
The test harness doesn't use the ID's at all (it seems to treat them as hidden values), and does lookups only by name.
This PR updates the testing infrastructure to have the underlying tables be a bit more flexibly named for the ORM, along with adding the example code for Django.
5b03c5b
to
4e56632
Compare
PTAL |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm! i just had another question but i'm fine going either way on it
Reviewable status: 0 of 15 files reviewed, 5 unresolved discussions (waiting on @jordanlewis, @rafiss, and @rohany)
python/django/cockroach_example/migrations/0001_initial.py, line 1 at r1 (raw file):
# Generated by Django 2.2.6 on 2019-10-02 18:35
just noticed this was generated -- should it be gitignored?
No, migrations are always checked into Django repositories. Does this repo have bors? bors r+ |
🔒 Permission denied Existing reviewers: click here to make rohany a reviewer |
F |
This PR updates the testing infrastructure to have the underlying tables
be a bit more flexibly named for the ORM, along with adding the example
code for Django.