Skip to content
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

Download_Mode for File #12017

Open
wants to merge 15 commits into
base: develop
Choose a base branch
from
Open

Conversation

GregoryTravis
Copy link
Contributor

@GregoryTravis GregoryTravis commented Jan 7, 2025

Implements Download_Mode policy for local files.

Checklist

Please ensure that the following checklist has been satisfied before submitting the PR:

  • The documentation has been updated, if necessary.
  • Screenshots/screencasts have been attached, if there are any visual changes. For interactive or animated visual changes, a screencast is preferred.
  • All code follows the
    Scala,
    Java,
    TypeScript,
    and
    Rust
    style guides. In case you are using a language not listed above, follow the Rust style guide.
  • Unit tests have been written where possible.
  • If meaningful changes were made to logic or tests affecting Enso Cloud integration in the libraries,
    or the Snowflake database integration, a run of the Extra Tests has been scheduled.
    • If applicable, it is suggested to paste a link to a successful run of the Extra Tests.

@GregoryTravis GregoryTravis marked this pull request as ready for review January 7, 2025 18:52
@@ -443,17 +444,18 @@ post (uri:(URI | Text)=(Missing_Argument.throw "uri")) (body:Request_Body=..Empt
- headers: The headers to send with the request. Defaults to an empty vector.
@uri (Text_Input display=..Always)
@headers Header.default_widget
download : (URI | Text) -> Writable_File -> HTTP_Method -> Vector (Header | Pair Text Text) -> File ! Request_Error | HTTP_Error
download (uri:(URI | Text)=(Missing_Argument.throw "uri")) file:Writable_File (method:HTTP_Method=..Get) (headers:(Vector (Header | Pair Text Text))=[]) =
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

API suggestion: what if we rename mode into replace_existing?

IMO it will be much clearer for the user.

If I do:

Data.download url1 file mode=..If_Not_Exists
Data.download url2 file mode=..If_Not_Exists

tbh I'd still expect the second expression to download. That is because I am now downloading a different file to that destination. So while the destination exists, as a user I would expect it to be overwritten, because I've changed the URL - e.g. I was working with a report from June relying on the cache to only download it once, but now I want to start working with reports from July. I change the URL and expect the file to get redownloaded even if it exists - because I expect the data is new. I even reset caches and am confused why I'm still seeing June data in the file.

I understand that this is not what this was designed for, but I think the above is a likely user scenario.

Now, if we rename the parameter to replace_existing, the code reads as:

Data.download url1 file replace_existing=..If_Not_Exists
Data.download url2 file replace_existing=..If_Not_Exists

Now it is obvious to me that the second statement will do nothing if the first one succeeded - because the file is redownloaded only if it didn't exist in the first place (regardless of the URL). And now that semantics (what is currently implemented) is completely clear when reading the calls.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

and regardless of parameter name - we need to update the method documentation to include it and ideally describe what the expected semantics is.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Given that we rely on the 'refresh' button to clear caches in many cases, we probably should add a note that this download method works only based on file existance/age and so refresh button does not affect it.

As a user I might expect the refresh button to ensure the file is redownloaded (whether that should work this way or not is up to discussion, I think current semantics are ok) - but with current semantics the refresh button just does nothing for download. I think it would be good if the documentation mentioned that, so that the user can know what to expect / can see that this is expected and not a bug if they get confused seeing that refresh button does nothing.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Renamed to replace_existing, and added documentation.

I am not sure about how the refresh button interacts with the Always option, and I cannot run the front end to find out, so I did not mention that.

@GregoryTravis GregoryTravis requested a review from radeusgd January 8, 2025 20:23
@GregoryTravis
Copy link
Contributor Author

Still need to update tests.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants