-
-
Notifications
You must be signed in to change notification settings - Fork 12
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
Add timestamps to downloaded files #514
base: main
Are you sure you want to change the base?
Conversation
This seems to be not working |
(cherry picked from commit 1841bb4)
It works now! |
This is really good. If folders also have a modification time, it might be worth to set it too, unless this is already done by the OS upon changing one of the children. That leaves links and webdav attachments, which I guess aren't as important (and in the case of the latter I don't even think it would be correct), up for your consideration. Also, you are free to add your user tag on the CHANGELOG if you desire. Overall great PR though. Seems to pass tests too. |
Unrelated, but if you are able to test #510 I would really appreciate it. |
Folder time is not set by changing children. I'll need to check if similar change will work.
I'll set up links, as it has
I checked some of my ultraDocumentBody but don't seem to have similar problem as original issue. I'll test if I can repeat this problem. |
1280fc8
to
047c213
Compare
html files still have no correct timestamps. |
(cherry picked from commit 302e1e4)
c1437ac
to
ddc21aa
Compare
… file timestamps accordingly, for folders
ddc21aa
to
d04ae42
Compare
I've added the timestamp whereever have mkdir. I think that's it? Please tell me if there still have anything missing. Build could be found here https://github.com/Ovler-Young/BlackboardSync/actions/runs/13300762864/job/37141668562 |
@@ -1,6 +1,7 @@ | |||
import logging | |||
from pathlib import Path | |||
from datetime import datetime | |||
import os |
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.
This import seems out of order here, better at the top
@@ -41,6 +43,11 @@ def write(self, path: Path, executor: ThreadPoolExecutor) -> None: | |||
for child in self.children: | |||
child.write(path, executor) | |||
|
|||
if self.modified_time: | |||
timestamp = self.modified_time.timestamp() | |||
path.touch(exist_ok=True) |
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.
Surely the folder exists by this point. So there is no use in touching it
if (self.modified_time and | ||
(self.handler is not None or self.body is not None)): | ||
timestamp = self.modified_time.timestamp() | ||
path.touch(exist_ok=True) |
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 think by this point path is guaranteed to exist (other than by destructive user action) so no need to touch
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.
Thank you for adding the folder, link and webdav modification times. However, there are some things I would like you to change. Most important is to not add unnecessary code. For instance, in document.py we are doing the os.utime operation when this can already be handled down the line by attachment.write. There are also some imports which seem out of order and one instance of a name not being consistent with other classes. Thank you for your help, however, and feel free to make the changes whenever you can.
@@ -37,6 +41,11 @@ def write(self, path: Path, executor: ThreadPoolExecutor) -> None: | |||
for attachment in self.attachments: | |||
attachment.write(path, executor) |
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 think it's better to do this in the write method, as it can just be handled by write_base
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.
So just pass it to attachment.write
@@ -31,7 +32,7 @@ def write(self, path: Path, executor: ThreadPoolExecutor) -> None: | |||
body = self.create_unix_body(self.url) | |||
path = path.with_suffix(".desktop") | |||
|
|||
super().write_base(path, executor, body) | |||
super().write_base(path, executor, body, self.modified_time) |
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.
This is exactly how it should be
@@ -1,4 +1,5 @@ | |||
from pathlib import Path | |||
import os |
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.
Again, this import seems better at the top
@@ -11,9 +12,10 @@ | |||
class Folder: | |||
"""Content of type `x-bb-folder`.""" | |||
|
|||
def __init__(self, _: BBCourseContent, api_path: BBContentPath, | |||
def __init__(self, coursecontent: BBCourseContent, api_path: BBContentPath, |
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.
Rename to content, as in other instances
@@ -28,6 +30,10 @@ def write(self, path: Path, executor: ThreadPoolExecutor) -> None: | |||
for child in self.children: | |||
child.write(path, executor) | |||
|
|||
if self.modified_time: |
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.
This is fine - since folder doesn't inherit from BStream or FStream
@@ -113,11 +113,12 @@ def __init__(self, link: Link, job: DownloadJob) -> None: | |||
self.valid = validate_webdav_response(self.stream, link.href, | |||
job.session.instance_url) | |||
|
|||
def write(self, path: Path, executor: ThreadPoolExecutor) -> None: | |||
def write(self, path: Path, | |||
executor: ThreadPoolExecutor, modified_time=None) -> None: |
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.
Lacking type datetime | None
@@ -1,5 +1,6 @@ | |||
import uuid | |||
import mimetypes | |||
from datetime import datetime |
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.
This import would be better with the other imports of the form 'from x import y', I usually order by length
Okay! I'll make chages, perhap during the Spring Break! |
Sounds good! |
Set downloaded files' modified time from API
This change preserves the content modification times reported by the Blackboard API instead of using local download time. Files will now show their actual last updated time from the learning platform.
Affected components:
Close #486