Skip to content

ENH: Assume TCK is open in binary mode #1142

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

Merged
merged 3 commits into from
Nov 18, 2022

Conversation

anibalsolon
Copy link
Collaborator

The current TCK code makes some conversions between text and binary when reading and writing, which should not be required: the file should be open in binary mode, as it contains binary information after the text-encoded header.

This PR addresses this by assuming the file handler is in binary mode.

As a minor change, it changes the logic on computing the data offset considering the length of the length representation (chicken-egg thing). It will always be at max +1 char to represent the length.

@codecov
Copy link

codecov bot commented Oct 5, 2022

Codecov Report

Base: 91.81% // Head: 91.81% // Decreases project coverage by -0.00% ⚠️

Coverage data is based on head (203ed0c) compared to base (a4c420d).
Patch coverage: 100.00% of modified lines in pull request are covered.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1142      +/-   ##
==========================================
- Coverage   91.81%   91.81%   -0.01%     
==========================================
  Files         101      101              
  Lines       12469    12463       -6     
  Branches     2239     2238       -1     
==========================================
- Hits        11449    11443       -6     
  Misses        693      693              
  Partials      327      327              
Impacted Files Coverage Δ
nibabel/streamlines/tck.py 99.49% <100.00%> (-0.02%) ⬇️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

Copy link
Contributor

@MarcCote MarcCote left a comment

Choose a reason for hiding this comment

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

LGTM. Thank you for adding the test checking when offset needs to be adjusted.
Got catch for the spurious -1 in out.count(":") > len(lines) - 1:!

Copy link
Member

@effigies effigies 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. Thanks @MarcCote for bumping this...

One small suggestion for readability, but feel free to push back.

@effigies
Copy link
Member

Failures are unrelated.

@effigies effigies merged commit c1f0d65 into nipy:master Nov 18, 2022
@anibalsolon anibalsolon deleted the fix/invalid_tck_handling branch November 19, 2022 01:10
@effigies effigies added this to the 5.0.0 milestone Jan 3, 2023
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.

3 participants