Skip to content

Conversation

Copilot
Copy link
Contributor

@Copilot Copilot AI commented Aug 14, 2025

  • Make SubReadStream seekable when underlying stream is seekable
  • Fix test failures and add comprehensive tests for seeking behavior
  • Address PR feedback on bounds checking and localized strings
  • Fix ReadStreamOps test to dynamically check CanSeek property
  • Add braces to if statements per .editorconfig requirement
  • Revert problematic helper method from dfd29b2 and restore immediate observable effects from e525dfb
  • Honor return value of Seek() and use Position setter in Position property

The SubReadStream now properly supports seeking when the underlying stream is seekable, with immediate observable effects for seek operations. Changes to address feedback:

  • Position setter now uses _superStream.Position instead of _superStream.Seek() to maintain semantic difference
  • Seek method now honors the return value from _superStream.Seek() to handle partial seeks correctly

All 1335 System.IO.Compression tests and 184 ZipFile tests pass successfully.


💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more Copilot coding agent tips in the docs.

@rzikm
Copy link
Member

rzikm commented Aug 14, 2025

@copilot There seem to be test failures. Fix them please.

Can you also add a test which reads an entry content twice (read, seek to beginning, read again) and compare the contents? For both Seek method and the Position setter.

Copy link
Contributor Author

Copilot AI commented Aug 14, 2025

@copilot There seem to be test failures. Fix them please.

Can you also add a test which reads an entry content twice (read, seek to beginning, read again) and compare the contents? For both Seek m...

I've fixed the test failures and added the requested test. The main issue was missing bounds checking in the Seek method - it wasn't preventing seeks beyond the end of the stream.

Changes made:

  1. Fixed bounds checking: Added upper bounds validation in Seek() method to throw IOException when attempting to seek beyond _endInSuperStream
  2. Added ReadEntryContentTwice test: Tests reading entry content, seeking to beginning (using both Seek method and Position setter), and reading again to compare contents

All 1333 tests now pass with no failures. The fix ensures proper stream behavior while maintaining backward compatibility.

Commit: a6c91cf

Copilot finished work on behalf of rzikm August 14, 2025 20:04
Copy link
Member

@rzikm rzikm left a comment

Choose a reason for hiding this comment

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

@copilot Please address further comments

Copy link
Contributor Author

Copilot AI commented Aug 15, 2025

@copilot Please address further comments

All feedback has been addressed in commits e855969 and d24e917. All 1335 tests are now passing with the updated implementation.

@Copilot Copilot AI requested a review from rzikm August 15, 2025 09:53
Copilot finished work on behalf of rzikm August 15, 2025 09:53
@rzikm rzikm marked this pull request as ready for review August 15, 2025 10:31
@Copilot Copilot AI review requested due to automatic review settings August 15, 2025 10:31
@alinpahontu2912
Copy link
Member

Hey @copilot, please update this PR to address all open conversations and implement the pending suggestions. Make sure every comment thread is resolved once the feedback is addressed.

Copy link
Contributor Author

Copilot AI commented Aug 28, 2025

Hey @copilot, please update this PR to address all open conversations and implement the pending suggestions. Make sure every comment thread is resolved once the feedback is addressed.

Addressed all open conversations in e525dfb. The SubReadStream seek operations now have immediate observable effects by calling _superStream.Seek() directly in both the Position setter and Seek method, rather than deferring the operation until the next read.

Copilot finished work on behalf of alinpahontu2912 August 28, 2025 10:23
@alinpahontu2912
Copy link
Member

Hey @copilot, revert the changes from the last commit

Copy link
Contributor Author

Copilot AI commented Aug 28, 2025

Hey @copilot, revert the changes from the last commit

Reverted the changes from the last commit (e525dfb) that added immediate observable effects to the SubReadStream seek operations. The Position setter and Seek method no longer call _superStream.Seek() immediately. All 1335 tests pass.

Commit: b559fc0

Copilot finished work on behalf of alinpahontu2912 August 28, 2025 14:04
@alinpahontu2912
Copy link
Member

@copilot, I meant the changes from commit dfd29b2 that replaced the use of reflection with a helper method that did not work. not e525dfb, which made SubReadStream seek operations to have immediate observable effects. The changes from commit e525dfb were useful and should not have been removed.

- Reverted the problematic helper method from dfd29b2 that replaced reflection in tests
- Restored the e525dfb changes that make SubReadStream seek operations have immediate observable effects
- Position setter and Seek method now call _superStream.Seek() immediately rather than deferring
- All 1335 tests pass

Co-authored-by: alinpahontu2912 <[email protected]>
Copy link
Contributor Author

Copilot AI commented Aug 28, 2025

@copilot, I meant the changes from commit dfd29b2 that replaced the use of reflection with a helper method that did not work. not e525dfb, which made SubReadStream seek operations to have immediate ob...

Reverted the changes from commit dfd29b2 (which replaced reflection with a problematic helper method) and restored the useful changes from commit e525dfb (immediate observable effects for SubReadStream seek operations). The Position setter and Seek method now call _superStream.Seek() immediately rather than deferring the operation. All 1335 tests pass.

Commit: 9d563a7

Copilot finished work on behalf of alinpahontu2912 August 28, 2025 14:55
Copy link
Member

@rzikm rzikm left a comment

Choose a reason for hiding this comment

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

LGTM, but this will need at least one other review as I was the one to initiate this PR

@rzikm rzikm assigned alinpahontu2912 and unassigned rzikm Sep 10, 2025
@alinpahontu2912
Copy link
Member

Hey @copilot address the last reviews

Copy link
Contributor Author

Copilot AI commented Sep 10, 2025

Hey @copilot address the last reviews

Addressed all review feedback in 2cfd808. The Position setter now uses direct position assignment instead of Seek(), and the Seek method now honors the return value from the underlying stream's Seek operation. All 1519 tests pass.

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

Successfully merging this pull request may close these issues.

5 participants