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

Have 'Segment.chunks' yield smaller 'Segment's #37

Merged
merged 5 commits into from
Sep 20, 2023

Conversation

bessman
Copy link
Contributor

@bessman bessman commented Sep 18, 2023

Per discussion in #35.

Three tests are failing so it wasn't quite as straight-forward as I'd hoped. I'll try to get them passing.

@bessman bessman marked this pull request as ready for review September 18, 2023 19:17
@bessman
Copy link
Contributor Author

bessman commented Sep 18, 2023

Tests are passing. I reverted the changes to _Chunk, since it's no longer needed except for compatibility.

Should Segment have a __len__ instead?

@bessman
Copy link
Contributor Author

bessman commented Sep 18, 2023

I'm working on updating outdated docstrings referring to Chunk.

@bessman bessman changed the title Have 'Segment.chunks' yield smaller smaller 'Segment's Have 'Segment.chunks' yield smaller 'Segment's Sep 18, 2023
@bessman
Copy link
Contributor Author

bessman commented Sep 18, 2023

One subtle change that comes with this is that Segment instances can now be immediately adjacent to each other. Previously, one could rely on data immediately before or after a Segment being null. Could that be a problem?

@eerimoq
Copy link
Owner

eerimoq commented Sep 18, 2023

Will think about that tomorrow.

@eerimoq
Copy link
Owner

eerimoq commented Sep 19, 2023

I think it's fine.

bincopy.py Outdated Show resolved Hide resolved
@bessman
Copy link
Contributor Author

bessman commented Sep 20, 2023

Went ahead and added Segment.__len__, taking word_size_bytes into account.

@eerimoq eerimoq merged commit 6d45975 into eerimoq:master Sep 20, 2023
5 checks passed
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