Replace os.path with pathlib.Path
#46
Conversation
krishnans2006
force-pushed
the
master
branch
13 times, most recently
from
c03ce92
to
090d035
Compare
There was a problem hiding this comment.
Amazing work - I appreciate this PR so much. As a side note, the many error-handling improvements and sanity checks are great - thank you so much for those.
I haven't tested these changes yet (there's a lot to test), but here are a few comments from a quick code review. Anything that involves testing is a note to myself (so I'll know what I should take a closer look at) - don't worry about those comments for now.
| @@ -170,14 +170,14 @@ def save_grader_file(self, grader_text: str) -> None: | |||
| self.grader_file.name = fname | |||
| self.save() | |||
|
|
|||
| fpath = os.path.join(settings.MEDIA_ROOT, self.grader_file.name) | |||
| fpath = settings.MEDIA_ROOT / self.grader_file.name | |||
There was a problem hiding this comment.
Need to test - this is slightly concerning.
There was a problem hiding this comment.
I'm not sure what is concerning here (again, I'm not against you testing it) :)
There was a problem hiding this comment.
Not the change itself - I'm just a bit sus of how this code block works.
| @@ -239,12 +234,15 @@ def save_file(self, submission_text: str) -> None: | |||
|
|
|||
| fpath = self.file_path | |||
|
|
|||
| os.makedirs(os.path.dirname(fpath), exist_ok=True) | |||
| if fpath is None: | |||
| raise ValueError("Cannot save file if file_path is not found") | |||
There was a problem hiding this comment.
This line is also a bit concerning and should be tested.
There was a problem hiding this comment.
If it works right now, it shouldn't make a difference (however, I don't discourage some testing!). The problem occurs because the file_path can be None if self.file is None. I just added this because pyright was complaining about fpath being None.
Same with similar exceptions being raised below.
There was a problem hiding this comment.
Yep, I see that. Just want to make sure fpath is never actually None to begin with.
| submission_path = submission.file_path | ||
|
|
||
| submission_wrapper_path = submission.wrapper_file_path | ||
|
|
||
| if submission_wrapper_path is None: | ||
| raise ValueError("File not provided") |
for more information, see https://pre-commit.ci
krishnans2006
force-pushed
the
master
branch
2 times, most recently
from
305ca99
to
ad43913
Compare
JasonGrace2282
added
enhancement
|
I think the approach taken here (replace |
See https://treyhunner.com/2018/12/why-you-should-be-using-pathlib/
Progress
os.path.dirname,os.path.join, andos.chmodwithpathlib.PathequivalentsPathobjects