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

Move write_bytes to be part of Vector. #3583

Merged
merged 8 commits into from
Jul 14, 2022

Conversation

jdunkerley
Copy link
Member

@jdunkerley jdunkerley commented Jul 13, 2022

Pull Request Description

Updates write_bytes API to be part of Vector and to conform to write APIs.

Important Notes

Ensures doesn't touch the file if an invalid byte array.

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 dist and ./run ide watch.

@jdunkerley jdunkerley requested review from 4e6 and radeusgd as code owners July 13, 2022 12:10
@jdunkerley jdunkerley requested a review from hubertp July 13, 2022 12:20
@jdunkerley jdunkerley force-pushed the wip/jd/vector-write_bytes-182309487 branch from e86551a to fbb7dc3 Compare July 13, 2022 13:10
Copy link
Member

@radeusgd radeusgd left a comment

Choose a reason for hiding this comment

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

Looks good to me, especially the thorough testing of all append/overwrite modes. Just a few minor comments above.

@jdunkerley jdunkerley added the CI: Ready to merge This PR is eligible for automatic merge label Jul 13, 2022
@mergify mergify bot merged commit 9578dc1 into develop Jul 14, 2022
@mergify mergify bot deleted the wip/jd/vector-write_bytes-182309487 branch July 14, 2022 11:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CI: Ready to merge This PR is eligible for automatic merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants