Skip to content

Update random-access-storage to 2.0.0#12

Merged
yoshuawuyts merged 1 commit intodatrs:masterfrom
jackjennings:update-random-access-storage-2
Apr 24, 2019
Merged

Update random-access-storage to 2.0.0#12
yoshuawuyts merged 1 commit intodatrs:masterfrom
jackjennings:update-random-access-storage-2

Conversation

@jackjennings
Copy link
Copy Markdown
Contributor

@jackjennings jackjennings commented Apr 20, 2019

Submitting this pull request because I need some help resolving an issue Update: resolved.

Checklist

  • tests pass
  • tests and/or benchmarks are included
  • documentation is changed or added

Context

This pull request is in regards to work on this issue to update usize to u64: datrs/random-access-storage#6

As this package would need to update to the upcoming 3.0.0 of random-access-storage, I'm attempting to first update to 2.0.0, which requires adding a handful of additional methods to the RandomAccess trait.

I've added len a is_empty, but truncate is troublesome. I assumed that I could make this implementation del(0, length), but this throws an error when the offset provided to del is zero. I'm unsure if this is a bug in del, or if I need to provide a more complex implementation of truncate.

thread 'can_truncate_lt' panicked at 'attempt to subtract with overflow', src/lib.rs:180:19

I've added failing tests for truncate based on the tests in random-access-disk.

dependabot attempted to make this change previously in this pull request: #9

Semver Changes

This would likely be a minor version change, as additional functionality is added, while existing functionality is unchanged.

@jackjennings
Copy link
Copy Markdown
Contributor Author

Also I was asked to choose what kind of pull request this was from the template. I assume this is supposed to be a label I add, but labels seem to be disabled for non-contributors.

@jackjennings jackjennings marked this pull request as ready for review April 20, 2019 22:45
Comment thread tests/test.rs Outdated
// file.truncate(0).unwrap();
// assert_eq!(file.is_empty().unwrap(), true);
// file.write(0, b"what").unwrap();
// assert_eq!(file.is_empty().unwrap(), false);
Copy link
Copy Markdown
Contributor Author

@jackjennings jackjennings Apr 20, 2019

Choose a reason for hiding this comment

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

These assertions are disabled, as they require truncate to work.

@yoshuawuyts
Copy link
Copy Markdown
Contributor

@jackjennings these changes look reasonable to me; agree we should probably figure out why truncate doesn't work, as your reasoning seems sound.

If you could make sure tests pass (cargo fmt is failing) then I think we should be good to merge!

@jackjennings jackjennings force-pushed the update-random-access-storage-2 branch from 1e2b210 to 499fef4 Compare April 23, 2019 14:48
@jackjennings
Copy link
Copy Markdown
Contributor Author

jackjennings commented Apr 23, 2019

@yoshuawuyts I rebased — changed the implementation of truncate to unimplemented! and removed the tests that were commented that depended on truncate. I'll opens a separate pull request for the implementation of del. Looking at the way the JavaScript version works give me some clues as to what the issue may be.

Copy link
Copy Markdown
Contributor

@yoshuawuyts yoshuawuyts left a comment

Choose a reason for hiding this comment

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

LGTM

@yoshuawuyts yoshuawuyts merged commit 76c0ebf into datrs:master Apr 24, 2019
@yoshuawuyts
Copy link
Copy Markdown
Contributor

v1.0.0 🎉

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