Skip to content

Conversation

Copy link
Contributor

Copilot AI commented Jan 23, 2026

  • Understand current implementation of _read_first_bytes
  • Replace _read_first_bytes with _file_startswith helper function
  • Update _content_is_retrieved to use the new function
  • Remove the old _read_first_bytes function
  • Create tests to validate the new implementation
  • Run linting and type checking
  • Run existing tests to ensure no regressions
  • Request code review and address feedback
  • Rename parameter s to string per review feedback
  • Final verification and security check

Summary

Successfully refactored _read_first_bytes to _file_startswith as requested in issue #166.

Changes:

  • Replaced _read_first_bytes(file_path, n=6) with _file_startswith(file_path, string)
  • New function takes a string parameter instead of integer, making the intent clearer
  • Eliminates hardcoded value 6 (which was len("/annex"))
  • Returns boolean directly instead of bytes for comparison
  • Updated call: _read_first_bytes(file_path=file_path, n=6) != b"/annex"not _file_startswith(file_path=file_path, string="/annex")

Testing:

✅ All custom tests pass (basic and real-world scenarios)
✅ Existing unit tests pass
✅ Linting passes (ruff)
✅ Type checking passes (mypy)
✅ No security issues (CodeQL)

Security Summary:

No security vulnerabilities detected in the changes.

Original prompt

This section details on the original issue you should resolve

<issue_title>Minor style: refactor _read_first_bytes</issue_title>
<issue_description>FWIW - I think it is a better workflow for us, when dealing with such small suggestions, to split them as separate issues 'to follow-up on' separately from the main PR since they aren't really core to the functionality (in that case, a core bug fix) and shouldn't block such merges

I have no problem with the suggestion below, and will do it in a separate PR

dang, you are right! It is a little surprising though. Anyways, I do not feel it is worth a dedicated function really here, if anything could be _file_startswith(file_path: Path, s: str) to make its purpose of living to do f.read(len(s)) == s thus avoiding here hardcoded value of 6 to match len("/annex"). but whatever -- let's just keep going

Originally posted by @yarikoptic in #165 (comment)
</issue_description>

<agent_instructions>This should be an easy one - replace _read_first_bytes with the direct startswith check</agent_instructions>

Comments on the Issue (you are @copilot in this section)


✨ Let Copilot coding agent set things up for you — coding agent works faster and does higher quality work when set up for your repo.

Copilot AI and others added 2 commits January 23, 2026 03:10
Co-authored-by: CodyCBakerPhD <51133164+CodyCBakerPhD@users.noreply.github.com>
Co-authored-by: CodyCBakerPhD <51133164+CodyCBakerPhD@users.noreply.github.com>
Copilot AI changed the title [WIP] Refactor style for read_first_bytes function Refactor _read_first_bytes to _file_startswith for clarity Jan 23, 2026
Copilot AI requested a review from CodyCBakerPhD January 23, 2026 03:14
@codecov
Copy link

codecov bot commented Jan 23, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 84.48%. Comparing base (76689e8) to head (807e1e2).
⚠️ Report is 2 commits behind head on main.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #287      +/-   ##
==========================================
+ Coverage   84.12%   84.48%   +0.35%     
==========================================
  Files          36       36              
  Lines        1392     1424      +32     
==========================================
+ Hits         1171     1203      +32     
  Misses        221      221              
Flag Coverage Δ
unittests 84.48% <100.00%> (+0.35%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines Coverage Δ
src/nwb2bids/_converters/_datalad_utils.py 100.00% <100.00%> (ø)
🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Co-authored-by: CodyCBakerPhD <51133164+CodyCBakerPhD@users.noreply.github.com>
@CodyCBakerPhD CodyCBakerPhD changed the title Refactor _read_first_bytes to _file_startswith for clarity Refactored _read_first_bytes to _file_startswith for clarity Jan 23, 2026
@CodyCBakerPhD CodyCBakerPhD changed the title Refactored _read_first_bytes to _file_startswith for clarity Refactored _read_first_bytes to _file_startswith for clarity of use Jan 23, 2026
@CodyCBakerPhD CodyCBakerPhD added patch Increment the patch version when merged internal Changes only affect the internal API minor Increment the minor version when merged and removed patch Increment the patch version when merged labels Jan 23, 2026
@CodyCBakerPhD CodyCBakerPhD marked this pull request as ready for review January 23, 2026 03:34
@CodyCBakerPhD CodyCBakerPhD enabled auto-merge (squash) January 23, 2026 04:03
@CodyCBakerPhD CodyCBakerPhD merged commit ba59fc1 into main Jan 23, 2026
29 of 55 checks passed
@CodyCBakerPhD CodyCBakerPhD deleted the copilot/refactor-read-first-bytes-style branch January 23, 2026 04:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

internal Changes only affect the internal API minor Increment the minor version when merged

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

Minor style: refactor _read_first_bytes

2 participants