fix: add missing type annotations, warn on ncdf deprecation - #32
Open
petercorke wants to merge 5 commits into
Open
fix: add missing type annotations, warn on ncdf deprecation#32petercorke wants to merge 5 commits into
petercorke wants to merge 5 commits into
Conversation
Found while adding type annotations to plot() -- the docstring lists 'frequency'/'cdf'/'ncdf' as the accepted type= values, but the actual dispatch also accepts 'pdf'/'probability'/'cf'/'cumulative'/'normalized' and does not handle 'ncdf' at all. Left type annotated as plain str rather than Literal for this reason -- reconciling docstring vs behavior is a separate decision, out of scope for an annotations-only pass.
Type annotations (finding 6): - Histogram.plot(): full signature was completely unannotated. Added types to every parameter and the return type. Deliberately kept the type= parameter name as-is despite shadowing the builtin -- it's a public keyword argument (hist.plot(type="pdf")), renaming it is a breaking change out of scope here. Left it typed as plain str rather than Literal (see tech-debt.md entry added alongside this commit -- the docstring's listed values don't match what the method actually accepts). - ImageRegionFeatures.py: __str__/__repr__ (x2 each) -> str - Sources.py: _open_reader() -> Any (conditionally-imported ROS reader type behind an optional dependency; Any matches the existing pattern used elsewhere in this file for the same reason) Deprecation warnings (finding 7, corrected from the original review -- the ImageCore.py "A" property version-drift claim didn't reproduce, both sides already said 2.0.0): - ImageWholeFeaturesMixin.ncdf (Image.ncdf) and Histogram.ncdf both had a .. deprecated:: 2.0.0 docstring note but neither ever called warnings.warn, unlike every other deprecated method in the codebase. Added the standard warning to both, with messages matching what each actually delegates to (hist().cdf for the Image-level property, plain .cdf for the Histogram-level one -- also fixed the second property's docstring, which incorrectly said "hist().cdf" despite self already being the Histogram instance). Added a regression test for the new Histogram.ncdf warning and updated the existing (already-passing) Image.ncdf test to also assert the warning. Verified genuinely: reverted just the two warnings.warn calls (via git stash on the source file only, keeping the tests), confirmed both fail with "DeprecationWarning not triggered", restored the fix, confirmed both pass. Full suite: 817 passed (816 + 1 net new test), 15 skipped, no regressions.
Not up to standards ⛔🔴 Issues
|
| Category | Results |
|---|---|
| ErrorProne | 1 high |
🟢 Metrics 0 complexity · 0 duplication
Metric Results Complexity 0 Duplication 0
NEW Get contextual insights on your PRs based on Codacy's metrics, along with PR and Jira context, without leaving GitHub. Enable AI reviewer
TIP This summary will be updated as you push new changes.
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
1 participant
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
Type annotations (finding 6)
Histogram.plot(): full signature was completely unannotated. Added types to every parameter and the return type. Kept thetype=parameter name as-is despite shadowing the builtin — it's public API (hist.plot(type="pdf")), renaming is a breaking change out of scope here. Left it typed as plainstrrather thanLiteral— its docstring's listed values don't match what the method actually accepts (logged as its owntech-debt.mdentry, found while doing this).ImageRegionFeatures.py: both__str__/__repr__pairs →strSources.py:_open_reader()→Any(conditionally-imported ROS reader type behind an optional dependency, matching the existing pattern used elsewhere in the file)Deprecation warnings (finding 7, corrected from the original review) — the original review's
ImageCore.pyA-property version-drift claim didn't reproduce (both sides already say2.0.0). The real gap:ImageWholeFeaturesMixin.ncdf(Image.ncdf) andHistogram.ncdfboth had a.. deprecated:: 2.0.0docstring note but neither ever calledwarnings.warn, unlike every other deprecated method in the codebase. Added the standard warning to both, with messages matching what each actually delegates to — also fixedHistogram.ncdf's docstring, which incorrectly said "usehist().cdf" despiteselfalready being theHistograminstance.Test plan
Histogram.ncdfwarning, updated the existingImage.ncdftest to also assert the warningwarnings.warncalls (source only, kept tests), confirmed both fail with "DeprecationWarning not triggered", restored the fix, confirmed both pass