Feature/refactor#20
Merged
Merged
Conversation
…-win type hints - Add helpers/rosdep_wrapper.py as the single typed boundary around rosdep2; lazy imports preserve existing test patches and sys.modules swaps - Route rosdep_validator and verify_rosdep_mapping through the wrapper - Add XmlElement TYPE_CHECKING alias in package_types.py for future lxml typing - Annotate remaining functions in cmake_parsers, find_launch_dependencies, workspace, and logger so those modules are 100% typed
…oint - Add type hints to all functions in validation_steps, pkg_xml_formatter, rosdep_validator, package_xml_validator, and verify_rosdep_mapping - Use XmlElement alias (now a proper TypeAlias) consistently for lxml elements; isolate untyped argcomplete import behind a single ignore - Widen LaunchDependencyStep.package_name to str | None to match formatter.get_package_name()'s return type - Initialize PackageXmlValidator.rosdep_validator to None up-front and narrow with explicit checks before passing to dependent steps
- Add [tool.mypy] config + dev extras in pyproject.toml (strict; per-module ignore_missing_imports for rosdep2, argcomplete, regex, helpers.*) - Add mypy hook to .pre-commit-config.yaml (CI picks it up automatically) - Fix latent bugs surfaced by mypy: None-text sorted() crashes and reused-name type drift in pkg_xml_formatter; shadowed except-var in verify_rosdep_mapping; export narrowing in BuildTypeExportStep - Fix verify-rosdep-map hook to invoke as a module so the new rosdep_wrapper relative import resolves
- Narrow 'except Exception' in package_xml_validator.py and cmake_parsers.py to the actually-raised exceptions (lxml.etree.XMLSyntaxError, OSError, UnicodeDecodeError); fix latent bug where parse failures skipped the 'all_valid' aggregation. - Make 'CMAKE_KEYS_NO_ROSDEP' extensible via repeatable '--ignore-cmake-key' CLI flag. The built-in defaults (Threads, OpenMP, ament_cmake) are always merged in, never replaced. - Split the 898-line PackageXmlFormatter into a thin facade plus topic modules under helpers/formatter/: constants, dependency_queries, mutations, indentation, structural_checks. Public API of PackageXmlFormatter is unchanged; all 12 call sites in validation_steps.py and the 3 in package_xml_validator.py work without modification. - Document the new flag in Readme.md.
- Narrow broad 'except Exception' in rosdep_validator, verify_rosdep_mapping, and workspace; delete dead bare except after the narrow catch in 'can_resolve'. Top-level CLI guard in verify_rosdep_mapping keeps 'except Exception' (documented; tests assert permissive behaviour). - Expose 'rospkg.ResourceNotFound' via rosdep_wrapper; whitelist 'rospkg.*' in mypy. - Delete dead 'check_count' wrap in package_xml_validator. - Split the 750-line validation_steps.py into a package, one step per file. Public names re-exported from __init__.py — call sites unchanged. - Enable branch coverage in tests/coverage_test.py. New baseline: 89% line / 89% branch (was 92% / 0% — branch wasn't tracked). - Register 'validation_steps' and 'formatter' subpackages in pyproject.
- Delete dead _calculate_num_checks; num_checks is overwritten per-file from len(steps) anyway. Hardcoded base_checks=11 didn't match reality. - Remove redundant 'if result.changed: self.xml_valid = False'; document the changed→not-valid invariant on ValidationResult. - Extract _expected_build_type helper to flatten the 15-line conditional in BuildTypeExportStep. - Split add_dependencies into _find_insert_position and _fix_surrounding_whitespace. - Enable branch coverage in CI (--branch flag). - Delete commented-out ET.indent line.
- Rename Readme.md → README.md so pyproject's 'readme = README.md' resolves on case-sensitive filesystems. - Delete dead check_with_xmllint parameter from PackageXmlFormatter; the only call site hardcoded it to False and the value was never read. - Switch library-path prints in workspace.py and find_launch_dependencies.py to module-level loggers
- Run a check_for_empty_lines + check_indentation pass after the step loop so auto-fill mutators always emit well-formed XML. - Add PackageType.UNKNOWN for manifest-only packages (no CMakeLists.txt and no setup.py); BuildToolDependStep skips with a warning instead of silently auto-filling ament_cmake. Updated test fixture to match. - Make encountered_unresolvable_error a local in check_and_format_files so reused validators don't carry stale critical-error state. - Load cmake_rosdep_map.yaml via importlib.resources — no more cwd dependency. - Use Path.read_text() in test_cmake_comparison.py:159 so the assertion message no longer leaks file handles.
- Add class docstrings to the seven *Step classes describing the rule, inputs, mutation behaviour, and opt-out flags. - Add an Architecture section to README.md mapping each module to its responsibility plus a note on adding a new step.
- Add CheckOutcome enum and check_element_occurrences_detailed. FormatterValidationStep now routes UNFIXABLE findings (missing required tags, diverging duplicates) to critical_errors instead of misreporting them as 'Corrected successfully'. - Regression test asserts the log output for every fixture in unfixable_pkgs/. - Fix inverted --skip-rosdep-key-validation help text. - Clarify README: drop the 'validates against the ROS 2 schema' claim; full XSD validation is out of scope.
…code) - XXE: pass no_network=True, resolve_entities=False to all lxml.XMLParser call sites (validator + test). - Deps: pin lower+upper bounds on all 7 runtime/dev dependencies in pyproject.toml; suppress S8565 (library, not application). - Complexity: split check_dependency_order into 3 helpers along the natural seams (collect → detect → reinsert). - Dead code: drop unused check_only param from check_for_non_existing_tags; remove always-true 'is not None' check in BuildToolDependStep error message.
Removes the try/except-ImportError shim from package_xml_validator, pkg_xml_formatter, and rosdep_validator. The fallback was unused (no in-repo caller invokes the modules with sys.path rooted inside the package); installed-package usage and the entry-point script both go through the package-relative path. Drops the now-obsolete 'helpers.*' mypy override.
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
pkg_xml_formatter.py(898 → 147 lines) andvalidation_steps.py(750 lines) are now packages with one concern per file. Public APIs unchanged.all_validbug fixed; dead bare-except inrosdep_validator.can_resolveremoved.PackageType.UNKNOWNfor manifest-only packages, no more silent misclassification asament_cmake.encountered_unresolvable_erroris local (no stale state across runs);verify_rosdep_mapping.pyis import-safe;Readme.md→README.mdsopyproject.tomlresolves on Linux.--ignore-cmake-key(repeatable) extends the built-in Threads/OpenMP/ament_cmake set.*Stepclasses; Architecture section in README; branch coverage enabled in CI.