-
Notifications
You must be signed in to change notification settings - Fork 0
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
Data architecture #49
base: main
Are you sure you want to change the base?
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.
Looks good to me! just a couple of questions and suggestions :)
date = db.Column(db.Date()) | ||
overall_mark = db.Column(db.String(56)) | ||
|
||
posting = db.relationship("Posting") |
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.
Why do you need the relationship and the foreign key?
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.
Good question! The relationship gives me access to the related objects pythonically, while the foreign key is necessary for the database architecture
app/models.py
Outdated
email_address = db.Column(db.String(120), unique=True) | ||
secondary_email_address = db.Column(db.String(120), unique=True) | ||
joining_date = db.Column(db.Date()) | ||
completed_fast_stream = db.Column(db.Boolean()) |
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 don't seem to refer overtly to the fast stream anywhere else e.g. calling the repo Graduate Rotator not 'Fast Stream rotator.
If you want the app to be a blueprint which can be used for other graduate schemes, then maybe calling the field completed_graduate_scheme
or completed_scheme
would be more appropriate?
If not, maybe calling the app 'Fast stream rotator' and being more explicit that it's only for the fast stream would make it clearer?
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.
Good spot - I'll drop this column
@@ -0,0 +1 @@ | |||
Generic single-database configuration. |
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.
Do readmes also need new lines at the end?
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.
Don't know 🤔
app/models.py
Outdated
first_name = db.Column(db.String(128)) | ||
last_name = db.Column(db.String(128)) | ||
email_address = db.Column(db.String(120), unique=True) | ||
secondary_email_address = db.Column(db.String(120), unique=True) | ||
joining_date = db.Column(db.Date()) | ||
completed_fast_stream = db.Column(db.Boolean()) |
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.
Would any of these need to be not Nullable? e.g. it seems you wouldn't want to initialise a candidate without a name and email?
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.
Both good catches!
requirements.txt
Outdated
gunicorn==20.0.4 | ||
identify==1.5.5 | ||
identify==1.5.2 |
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.
Just curious - why have some of the packages been brought down in version number?
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.
I suspect it's because another package relies on earlier versions of these packages
app/models.py
Outdated
description = db.Column(db.Text()) | ||
|
||
|
||
class SkillsMixin(object): |
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.
How will this be used?
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.
Good catch - I think I had a very over-complicated plan for this but since I don't currently use it I'll bin it and bring it back if necessary
migrate = Migrate() | ||
|
||
|
||
class IdModel(object): |
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.
Didn't know you could do this! Just read the docs though and v cool :)
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.
Isn't it! Saves on some boilerplating
73cf4b0
to
5f0da03
Compare
This is still only an alpha so for the moment I'm only using an SQLite database. It may be a bit annoying later, but it should be enough to get the architecture done.
This commit is the initial design and migration for the data architecture
A diagram, both as a png and UML, of the proposed database structure
5f0da03
to
c12e427
Compare
This is an initial attempt to form a working data architecture for this project. The aim is:
Posting
, which is a combination of a specificRole
,Candidate
, and point in time