-
Notifications
You must be signed in to change notification settings - Fork 5
Pushing course data to DB & Added logging #288
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
Conversation
|
Here's the code health analysis summary for commits Analysis Summary
|
RafaelCenzano
left a comment
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.
Overall looking good check the few comments I added. I'd also recommend using ruff, it should be installed in the venv. you can run with ruff check to see issues. ruff format should help fix most lint errors.
db_init.py
Outdated
| import sys | ||
| import requests | ||
|
|
||
| from sqlalchemy import create_engine |
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.
Echoing the ruff comment, this should be removed since it's not needed/used
db_init.py
Outdated
|
|
||
| if new_courses: | ||
| session.add_all(new_courses) | ||
| session.commit() |
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.
for session.commit, move that into the if statement since we only need it in the if statement
db_init.py
Outdated
| with app.app_context(): | ||
| db.create_all() | ||
|
|
||
| json_url = "https://raw.githubusercontent.com/quacs/quacs-data/master/semester_data/202409/catalog.json" |
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.
can you update this function to intake the URL as an argument, should check if sys.argv[2] is present (add logic to confirm extra arguments exist). Add logic to confirm that it's a URL.
|
You should also try to merge from main too just so your code is up to date before merging |
RafaelCenzano
left a comment
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 addressed the major errors, I think it should be resolved. I'm not sure if the checks are updated. |
Description & Fixes # (issue)
Pushing course data to DB:
In db_init.py, added an argument for addCourses to add courses from json endpoint to database. Updated functions and addCourses code. I found a case in which course_code has a string of length 9.
Course data can now be pushed to the Database, course_code only accepts string of length 8 to account for course_code errors. Course data is only added to the DB, if not found in the DB.
Added logging:
Added logging with app.logger that logs information for initialization and functions in init.py file,
Type of change
How Has This Been Tested?
In powershell:
Run python db_init.py addCourses
In wsl:
Run python3 db_init.py addCourses
Checklist: