-
Notifications
You must be signed in to change notification settings - Fork 58
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
feat: Add form and question models #25
Conversation
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.
file line ending issues are in every file, make sure to check your IDE settings
osp/admin.py
Outdated
@@ -3,5 +3,13 @@ | |||
from osp.models import * |
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.
No! Please don't. Never do this
osp/models/__init__.py
Outdated
from osp.models.question import Question | ||
from osp.models.abstract_timestamp import AbstractTimestamp | ||
from osp.models.field import * |
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.
Same here
osp/models/abstract_timestamp.py
Outdated
) | ||
|
||
class Meta: | ||
|
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.
extra line, please remove
osp/models/abstract_timestamp.py
Outdated
Abstract model for Created Time and Updated Time fields | ||
""" | ||
|
||
created_on = models.DateTimeField( |
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.
single param, same line should suffice
osp/models/question.py
Outdated
|
||
from osp.models.abstract_timestamp import AbstractTimestamp | ||
|
||
SA = 'char' |
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.
Create a separate directory for utilities, This new dir should contain all the abstract or generic models, functions, choices, views and manager.
osp/models/form.py
Outdated
|
||
def __str__(self): | ||
|
||
return f'{self.name}' |
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.
F formatting for a single param doesn't makes sense
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.
Without f the display is {self.name}
. I feel it is needed!
osp/models/form.py
Outdated
description = models.TextField( | ||
blank=True | ||
) | ||
created_on = models.DateTimeField( |
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.
You are already inheriting AbstractTimestamp
. no need to add these 2 fields again
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.
Please make the changes suggested by @Monal5031. I don't see any other changes.
Description
Fixes #17
Type of Change:
Code/Quality Assurance Only
How Has This Been Tested?
The models are created and migrations run:
Checklist:
Code/Quality Assurance Only