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

Review File/Data read and read_text warnings #5799

Merged
merged 19 commits into from
Mar 6, 2023

Conversation

radeusgd
Copy link
Member

@radeusgd radeusgd commented Mar 2, 2023

Pull Request Description

Closes #5113

Fixes a bug where read-only files would be overwritten if File.write was used in backup mode, and added tests to avoid such regression. To implement it, introduced a is_writable property on File.

Important Notes

Checklist

Please include the following checklist in your PR:

  • The documentation has been updated if necessary.
  • All code conforms to the
    Scala,
    Java,
    and
    Rust
    style guides.
  • All code has been tested:
    • Unit tests have been written where possible.
    • If GUI codebase was changed: Enso GUI was tested when built using BOTH
      ./run ide build and ./run ide watch.

@radeusgd radeusgd requested a review from jdunkerley as a code owner March 2, 2023 15:46
@radeusgd radeusgd self-assigned this Mar 2, 2023
@jdunkerley jdunkerley requested a review from GregoryTravis March 2, 2023 17:39
@radeusgd radeusgd added the CI: No changelog needed Do not require a changelog entry for this PR. label Mar 2, 2023
@radeusgd radeusgd force-pushed the wip/radeusgd/5113-data-read-review-warnings branch from e94f77a to 5a4af70 Compare March 2, 2023 18:45
@radeusgd radeusgd added the CI: Ready to merge This PR is eligible for automatic merge label Mar 2, 2023

## Indicates that the given file is corrupted, i.e. the data it contains
is not in the expected format.
Corrupted_Format (file : File) (message : Text) (cause : Any | Nothing = Nothing)
Copy link
Contributor

Choose a reason for hiding this comment

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

Does Any not include Nothing? Just curious.

Copy link
Member Author

Choose a reason for hiding this comment

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

It does, you are correct.

We sometimes use the Any | Nothing pattern to reinforce that a Nothing value is allowed here and it has some a bit special meaning.

How I'd read this signature is that the cause may or may not be present (| Nothing) and it may be of Any type (if it's present).

From typelevel perspective, just Any would suffice, but this is supposed to serve as kind of documentation.

I'm open to revising this approach though.

Copy link
Contributor

Choose a reason for hiding this comment

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

No I think it makes sense to write it this way.

@radeusgd radeusgd force-pushed the wip/radeusgd/5113-data-read-review-warnings branch from 9926016 to 29f7e61 Compare March 3, 2023 14:22
@radeusgd radeusgd force-pushed the wip/radeusgd/5113-data-read-review-warnings branch from 11eb902 to adf499b Compare March 3, 2023 16:22
@radeusgd radeusgd linked an issue Mar 4, 2023 that may be closed by this pull request
@mergify mergify bot merged commit 2d29456 into develop Mar 6, 2023
@mergify mergify bot deleted the wip/radeusgd/5113-data-read-review-warnings branch March 6, 2023 03:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CI: No changelog needed Do not require a changelog entry for this PR. CI: Ready to merge This PR is eligible for automatic merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

File/Data read and read_text warnings
4 participants