-
Notifications
You must be signed in to change notification settings - Fork 171
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
refactor: csv loader code implementation #4529
Conversation
a2949c8
to
a32c714
Compare
course_discovery/apps/course_metadata/data_loaders/csv_loader.py
Outdated
Show resolved
Hide resolved
course_discovery/apps/course_metadata/data_loaders/csv_loader.py
Outdated
Show resolved
Hide resolved
82c4fe7
to
563a4a3
Compare
course_discovery/apps/course_metadata/data_loaders/csv_loader.py
Outdated
Show resolved
Hide resolved
course_discovery/apps/course_metadata/data_loaders/csv_loader.py
Outdated
Show resolved
Hide resolved
50e160b
to
d797485
Compare
course_discovery/apps/course_metadata/data_loaders/csv_loader.py
Outdated
Show resolved
Hide resolved
@@ -438,6 +453,16 @@ def _log_ingestion_error(self, error_code, message): | |||
logger.error(message) | |||
self._register_ingestion_error(error_code, message) | |||
|
|||
@classmethod | |||
def clear_caches(cls): |
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 we need to clear the caches?
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.
Because functools caches are global caches instead of instance-level caches, I believe it's better to clear the caches after a job has been completed. If we don't do that, we would need to explicitly clear them in testsuites.
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.
If the only usage is in the tests, maybe add a comment indicating that.
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.
It's kind of used for tests as well, it's better to clear all the method-level caches after the execution of the job therefore I called this at the end of ingest method
@@ -438,6 +453,16 @@ def _log_ingestion_error(self, error_code, message): | |||
logger.error(message) | |||
self._register_ingestion_error(error_code, message) | |||
|
|||
@classmethod | |||
def clear_caches(cls): |
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.
If the only usage is in the tests, maybe add a comment indicating that.
30ef3be
to
ba6fbbf
Compare
ba6fbbf
to
fc7e7b4
Compare
PROD-4238
This PR refactors the CSV loader code to improve readability, making it cleaner and easier to follow.