Code quality review: final cleanup and mypy scope (#79, #81)

.github/workflows/ci.yml

@@ -31,9 +31,7 @@ jobs:
 
       - name: Type-check parser boundaries
         if: matrix.python-version == '3.12'
-        run: >-
-          uv run --frozen mypy fit_tool/fit_file.py fit_tool/fit_file_stream.py fit_tool/exceptions.py
-          --follow-imports=skip --allow-untyped-defs --allow-incomplete-defs --no-strict-optional
+        run: uv run --frozen mypy
 
       - name: Build package
         run: uv build

AGENTS.md

@@ -40,11 +40,11 @@ the full suite and `uv run gen-profile`. For a published package, install with
 
 The CI baseline is a package build plus the full test suite on Python 3.9 through 3.15. Before finishing a normal code change, run the narrowest relevant tests and then the full suite when practical. Run `uv build` for packaging, entry-point, dependency, or metadata changes.
 
-**Ruff is a CI gate** on the Python 3.12 job (`ruff check fit_tool`). Mypy is configured in `pyproject.toml` and runs in a limited form in CI; core modules are not all under strict mypy yet. Locally:
+**Ruff is a CI gate** on the Python 3.12 job (`ruff check fit_tool`). Mypy is configured in `pyproject.toml` and runs in a limited form in CI; core modules are not all under strict mypy yet. The checked set lives in `[tool.mypy].files`; run `uv run mypy` (no path arguments) so the config applies. Locally:
 
 ```bash
 uv run ruff check fit_tool
-uv run mypy fit_tool
+uv run mypy
 ```
 
 Do not update `uv.lock` unless dependencies or dependency groups intentionally changed. Use `uv lock` or an appropriate `uv add`/`uv remove` command for intentional dependency changes; never edit the lock file by hand.

docs/FIT_CONFORMANCE_DESIGN.md

@@ -44,7 +44,7 @@ public-claims pass that keeps this table and `docs/CAPABILITY_BOUNDARY.md` hones
 | --- | --- | --- | --- |
 | Docs / status truth vs main | Phase 0 | **A** SHA-13 | **Done** (re-synced at Stage 5 / L) |
 | Fixture / golden corpus for protocol edges | Phase 0 | **B** SHA-14 | **Done** inventory + constructive goldens; expand as follow-up only |
-| Full component / accumulator coverage beyond `_KNOWN_COMPONENTS` | Phase 3 | **C** SHA-15 | **Done** main-field registry + nested + rollover; active-subfield components via D |
+| Full component / accumulator coverage beyond `PROFILE_COMPONENTS` | Phase 3 | **C** SHA-15 | **Done** main-field registry + nested + rollover; active-subfield components via D |
 | Subfield resolution (type / scale / units / components) | Phase 3 | **D** SHA-16 | **Done** runtime match + PROFILE ambiguity ERROR |
 | Unknown field ids on known messages (decode retain + raw bytes) | Phase 3 | **E** SHA-17 | **Done** `UnknownField` + `raw_bytes` on main path |
 | Post-edit PRESERVATION (edited files, dirty records) | Phase 4 / PRESERVATION level | **F** SHA-18 | **Done** per-record dirty + mixed encode; opt-in PRESERVATION findings |

fit_tool/compatibility.py

@@ -23,6 +23,7 @@
 from fit_tool.profile.messages.field_description_message import FieldDescriptionMessage
 from fit_tool.record import Record, RecordHeader
 from fit_tool.wire.model import (
+    TIMESTAMP_FIELD_ID,
     FitDocument,
     FitSegment,
     RawDataRecord,
@@ -31,8 +32,6 @@
     RawRecord,
 )
 
-_TIMESTAMP_FIELD_ID = 253
-
 
 def project_header(raw: RawFileHeader) -> FitFileHeader:
     """Project a wire header into the existing FitFileHeader type."""
@@ -157,7 +156,7 @@ def project_data_record(
 
 def _apply_resolved_timestamp(message: DataMessage, timestamp: int) -> None:
     """Inject a reconstructed FIT date_time into field 253 when present."""
-    field = message.get_field(_TIMESTAMP_FIELD_ID)
+    field = message.get_field(TIMESTAMP_FIELD_ID)
     if field is None:
         return
     if field.size == 0:
@@ -199,10 +198,6 @@ def register_developer_field(
     fields_by_id[developer_field.field_id] = developer_field
 
 
-# Backward-compatible private alias used by earlier Stage 2 code / tests.
-_register_developer_field = register_developer_field
-
-
 def project_raw_record(
         raw: RawRecord,
         developer_fields_by_data_index: dict[int, dict[int, DeveloperField]],
@@ -253,21 +248,3 @@ def project_document(document: FitDocument) -> list[Record]:
     for segment in document.segments:
         records.extend(project_segment(segment, accumulators=accumulators))
     return records
-
-
-def project_records(records: list[RawRecord]) -> list[Record]:
-    """Project a sequence of raw records (used by unit tests)."""
-    segment = FitSegment(
-        header=RawFileHeader(
-            header_size=14,
-            protocol_version=0x20,
-            profile_version=0,
-            records_size=0,
-            data_type=b'.FIT',
-            crc=None,
-            source_offset=0,
-            source_bytes=b'',
-        ),
-        records=list(records),
-    )
-    return project_segment(segment)

fit_tool/components.py

@@ -22,9 +22,6 @@
     PROFILE_COMPONENTS,
 )
 
-# Back-compat alias: historical name used by docs and early tests.
-_KNOWN_COMPONENTS = PROFILE_COMPONENTS
-
 
 def components_for_field(
         global_id: int,
@@ -238,7 +235,6 @@ def expand_field(source: Field) -> None:
 __all__ = [
     'PROFILE_COMPONENT_SOURCE_COUNT',
     'PROFILE_COMPONENTS',
-    '_KNOWN_COMPONENTS',
     'components_for_field',
     'expand_message_components',
     'registry_coverage',

fit_tool/encode.py

@@ -23,12 +23,11 @@
 from fit_tool.definition_message import DefinitionMessage
 from fit_tool.exceptions import FitEncodingError
 from fit_tool.record import Record, RecordHeader
+from fit_tool.wire.model import TIMESTAMP_FIELD_ID
 
 if TYPE_CHECKING:
     from fit_tool.fit_file import FitFile
 
-_TIMESTAMP_FIELD_ID = 253
-
 
 class EncodeMode(str, Enum):
     """How :meth:`~fit_tool.fit_file.FitFile.to_bytes` rebuilds the FIT stream."""
@@ -37,11 +36,6 @@ class EncodeMode(str, Enum):
     CANONICAL = 'canonical'
 
 
-# Sentinel so an explicit ``check_crc=True`` is distinguishable from an omitted
-# kwarg (a bare default of True cannot override ``EncodeOptions(check_crc=False)``).
-_CHECK_CRC_UNSET: object = object()
-
-
 def _resolve_mode(
         *,
         mode: EncodeMode | str | None,
@@ -79,8 +73,8 @@ def resolve_encode_options(
         options: EncodeOptions | None = None,
         mode: EncodeMode | str | None = None,
         preserve: bool | None = None,
-        strict: bool = False,
-        check_crc: bool | object = _CHECK_CRC_UNSET,
+        strict: bool | None = None,
+        check_crc: bool | None = None,
 ) -> EncodeOptions:
     """Resolve kwargs and an optional :class:`EncodeOptions` into one value.
 
@@ -91,29 +85,15 @@ def resolve_encode_options(
     3. ``options.mode`` when an options object is supplied
     4. Default PRESERVE
 
-    Explicit kwargs win over the supplied options object; omitted kwargs keep
-    the options object's values. ``strict=True`` forces CANONICAL (see
-    :class:`EncodeOptions`).
+    Explicit ``strict`` / ``check_crc`` values override the supplied options
+    object; omitted (``None``) values keep it. ``strict=True`` forces CANONICAL
+    (see :class:`EncodeOptions`).
     """
-    check_crc_explicit = check_crc is not _CHECK_CRC_UNSET
-    check_crc_value = True if not check_crc_explicit else bool(check_crc)
-
-    if options is None:
-        return EncodeOptions(
-            mode=_resolve_mode(mode=mode, preserve=preserve),
-            strict=strict,
-            check_crc=check_crc_value,
-        )
-
-    if mode is None and preserve is None and not strict and not check_crc_explicit:
-        return options
-
-    # `preserve` stays a legacy alias for the no-options path; an explicit
-    # `mode` still wins over it.
+    base = options or EncodeOptions()
     return EncodeOptions(
-        mode=_resolve_mode(mode=mode, preserve=None, default=options.mode),
-        strict=strict or options.strict,
-        check_crc=check_crc_value if check_crc_explicit else options.check_crc,
+        mode=_resolve_mode(mode=mode, preserve=preserve, default=base.mode),
+        strict=base.strict if strict is None else strict,
+        check_crc=base.check_crc if check_crc is None else check_crc,
     )
 
 
@@ -139,7 +119,7 @@ def expand_compressed_header(record: Record) -> RecordHeader:
 
 def can_expand_compressed_to_normal(message: DataMessage) -> bool:
     """True when field 253 is on the definition so a normal header can carry the ts."""
-    return definition_includes_field(message.definition_message, _TIMESTAMP_FIELD_ID)
+    return definition_includes_field(message.definition_message, TIMESTAMP_FIELD_ID)
 
 
 def encode_record_projected(

fit_tool/field.py

@@ -311,9 +311,9 @@ def calculate_size(self) -> int:
 
     def get_encoded_value_from_bytes(self, bytes_buffer: bytes, offset: int = 0, endian: Endian = Endian.LITTLE):
         if self.base_type == BaseType.STRING:
-            length = len(bytes_buffer) - 1 - offset
-            value, = struct.unpack_from(f'{length}s', bytes_buffer, offset)
-            return value.decode('utf-8')
+            end = offset + self.size if self.size else len(bytes_buffer)
+            raw = bytes_buffer[offset:end]
+            return raw.split(b'\x00', 1)[0].decode('utf-8')
 
         endian_symbol = '<' if endian == Endian.LITTLE else '>'
 

fit_tool/fit_file.py

@@ -9,7 +9,6 @@
 
 from fit_tool.decoder import FitDecoder
 from fit_tool.encode import (
-    _CHECK_CRC_UNSET,
     EncodeMode,
     EncodeOptions,
     apply_strict_precheck,
@@ -19,7 +18,7 @@
 from fit_tool.exceptions import FitCRCError, FitEncodingError
 from fit_tool.fit_file_header import FitFileHeader
 from fit_tool.record import Record
-from fit_tool.utils.crc import crc16
+from fit_tool.utils.crc import crc16_chunks
 from fit_tool.utils.logging import logger
 from fit_tool.wire.encoder import encode_document, encode_document_mixed
 from fit_tool.wire.model import FitDocument
@@ -180,11 +179,11 @@ def from_bytes(
 
     def to_bytes(
             self,
-            check_crc: bool | object = _CHECK_CRC_UNSET,
+            check_crc: bool | None = None,
             *,
             preserve: bool | None = None,
             mode: EncodeMode | str | None = None,
-            strict: bool = False,
+            strict: bool | None = None,
             options: EncodeOptions | None = None,
     ) -> bytes:
         """Serialize this file.
@@ -311,14 +310,11 @@ def _to_bytes_projected(self, *, options: EncodeOptions) -> bytes:
                 self.header.protocol_version, self.header.profile_version, records_size
             )
 
-        calculated_crc = 0
-        bytes_buffer = bytearray()
-        buffer = self.header.to_bytes()
-        calculated_crc = crc16(buffer, crc=calculated_crc)
-        bytes_buffer.extend(buffer)
+        header_bytes = self.header.to_bytes()
+        calculated_crc = crc16_chunks([header_bytes, *record_buffers])
+        bytes_buffer = bytearray(header_bytes)
 
         for buffer in record_buffers:
-            calculated_crc = crc16(buffer, crc=calculated_crc)
             bytes_buffer.extend(buffer)
 
         if self._crc is None:
@@ -375,7 +371,7 @@ def to_file(
             *,
             preserve: bool | None = None,
             mode: EncodeMode | str | None = None,
-            strict: bool = False,
+            strict: bool | None = None,
     ) -> None:
         with open(path, 'wb') as file_object:
             file_object.write(

fit_tool/fit_file_builder.py

@@ -7,7 +7,7 @@
 from fit_tool.fit_file_header import FitFileHeader
 from fit_tool.message import Message
 from fit_tool.record import Record
-from fit_tool.utils.crc import crc16
+from fit_tool.utils.crc import crc16_chunks
 from fit_tool.validation import (
     STRICT_LEVELS,
     validate_definition,
@@ -24,10 +24,7 @@ def calc_records_size(records: list[Record]) -> int:
 
 
 def calc_crc(header: FitFileHeader, records: list[Record]) -> int:
-    crc = crc16(header.to_bytes())
-    for record in records:
-        crc = crc16(record.to_bytes(), crc=crc)
-    return crc
+    return crc16_chunks([header.to_bytes(), *(record.to_bytes() for record in records)])
 
 
 class FitFileBuilder:

fit_tool/tests/test_coverage_edges.py

@@ -156,10 +156,24 @@ def test_raw_header_and_file_header_size(self):
 
 
 class TestCompatibilityEdges(unittest.TestCase):
-    def test_project_records_empty(self):
-        from fit_tool.compatibility import project_records
+    def test_project_segment_empty(self):
+        from fit_tool.compatibility import project_segment
+        from fit_tool.wire.model import FitSegment
 
-        self.assertEqual(project_records([]), [])
+        segment = FitSegment(
+            header=RawFileHeader(
+                header_size=14,
+                protocol_version=0x20,
+                profile_version=0,
+                records_size=0,
+                data_type=b'.FIT',
+                crc=None,
+                source_offset=0,
+                source_bytes=b'',
+            ),
+            records=[],
+        )
+        self.assertEqual(project_segment(segment), [])
 
     def test_register_developer_field_missing_metadata(self):
         from fit_tool.compatibility import register_developer_field

fit_tool/tests/test_encode_policies.py

@@ -54,6 +54,32 @@ def test_mode_string(self):
         opts = resolve_encode_options(mode='canonical')
         self.assertEqual(opts.mode, EncodeMode.CANONICAL)
 
+    def test_preserve_overrides_options_mode(self):
+        opts = resolve_encode_options(
+            options=EncodeOptions(mode=EncodeMode.CANONICAL),
+            preserve=True,
+        )
+        self.assertEqual(opts.mode, EncodeMode.PRESERVE)
+
+    def test_mode_overrides_preserve_with_options(self):
+        opts = resolve_encode_options(
+            options=EncodeOptions(mode=EncodeMode.PRESERVE),
+            mode=EncodeMode.CANONICAL,
+            preserve=True,
+        )
+        self.assertEqual(opts.mode, EncodeMode.CANONICAL)
+
+    def test_explicit_strict_false_overrides_options_strict(self):
+        opts = resolve_encode_options(options=EncodeOptions(strict=True), strict=False)
+        self.assertFalse(opts.strict)
+
+    def test_omitted_kwargs_inherit_options(self):
+        base = EncodeOptions(mode=EncodeMode.CANONICAL, strict=True, check_crc=False)
+        opts = resolve_encode_options(options=base)
+        self.assertIs(opts.mode, EncodeMode.CANONICAL)
+        self.assertTrue(opts.strict)
+        self.assertFalse(opts.check_crc)
+
 
 class TestPreserveModeInvariants(unittest.TestCase):
     def test_untouched_identity(self):
@@ -63,6 +89,15 @@ def test_untouched_identity(self):
         # Legacy alias
         self.assertEqual(fit.to_bytes(preserve=True), raw)
 
+    def test_explicit_preserve_overrides_options_mode(self):
+        raw = build_record_with_unknown_field(heart_rate=101)
+        fit = FitFile.from_bytes(raw)
+        out = fit.to_bytes(
+            options=EncodeOptions(mode=EncodeMode.CANONICAL),
+            preserve=True,
+        )
+        self.assertEqual(out, raw)
+
     def test_dirty_unknown_survives_preserve(self):
         raw = build_record_with_unknown_field(
             heart_rate=100, unknown_field_id=250, unknown_value=0xBEEF,

fit_tool/utils/crc.py

@@ -7,6 +7,8 @@
 
 from __future__ import annotations
 
+from collections.abc import Iterable
+
 CRC_TABLE = (
     0x0000, 0xCC01, 0xD801, 0x1400, 0xF001, 0x3C00, 0x2800, 0xE401,
     0xA001, 0x6C00, 0x7800, 0xB401, 0x5000, 0x9C01, 0x8801, 0x4400,
@@ -33,3 +35,10 @@ def crc16(buffer, crc=0):
         crc = (crc >> 4) & 0x0FFF
         crc = crc ^ tmp ^ CRC_TABLE[(byte_char >> 4) & 0xF]
     return crc
+
+
+def crc16_chunks(chunks: Iterable[bytes], crc: int = 0) -> int:
+    """CRC-16 over a sequence of byte buffers, in order."""
+    for chunk in chunks:
+        crc = crc16(chunk, crc=crc)
+    return crc