You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
{{ message }}
Repository navigation
fix(map): only Winkel Tripel reads +lat_1, as the reader does - #1822
Follow-up to #1781, which merged while this last review fix was on its way. Projector::new checked +lat_1 for every world projection it computes, so it refused a Natural Earth or Eckert IV definition with a +lat_1 that the reader (WorldProjection::from_proj4) accepts and ignores, as PROJ does. The map now reads +lat_1 for Winkel Tripel only, so the two agree on a definition.
Reachability is low today: the map takes a preset, an EPSG code or an ESRI code from its table, and none gives these projections a +lat_1. Raised by the review of #1781 (thread on crates/map/src/projection.rs).
Testing
cargo clippy -p rustgis-map --all-targets -- -D warnings and cargo test -p rustgis-map --lib (295 passed) on Linux. No new test: no map code reaches the branch.
UI evidence
No visible UI change.
Summary by CodeRabbit
Bug Fixes
Winkel Tripel projections now reject invalid reference latitudes, including values outside the permitted range.
Other world projections no longer apply a supplied reference latitude, keeping their projection behavior consistent.
Projector::new checked +lat_1 for every world projection, so it refused
a Natural Earth or Eckert IV definition the reader accepts. No preset or
code gives one today.
No actionable comments were generated in the recent review.
Walkthrough
The world-projection constructor now parses and validates +lat_1 only for Winkel Tripel. Other world projections use lat1 = 0, regardless of a +lat_1 value in the definition.
Changes: Projection latitude handling
Layer / File(s)
Summary
Constructor latitude handling crates/map/src/projection.rs
The constructor validates supplied +lat_1 values for Winkel Tripel. For other world projections, it sets lat1 to 0 without parsing the parameter.
opengeos/RustGIS#1781: That PR defines the reader behavior that this change applies in Projector::new.
Suggested reviewers:bearinmindcat
Merge Risk: Minimal · up to 24df2
This change makes the map accept Natural Earth and Eckert IV definitions carrying +lat_1, as the reader already does. The only change in behavior is for definitions that previously failed with an error, and no current preset reaches it. There is no merge-blocking risk.
Pre-merge checks: 4
Check name
Status
Explanation
Title check
The title names the actual change: Projector::new now reads +lat_1 only for Winkel Tripel, matching the reader. The summary and objectives describe the same change. The title is 61 characters and 10 w…
Description check
The description follows the template sections: Summary, Testing, UI evidence, and Related issue. The Summary explains the change and its reason. Testing lists the clippy and test commands, the 295 pas…
Linked Issues check
Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check
Check skipped because no linked issues were found for this pull request.
Full details: Title check
Explanation
The title names the actual change: Projector::new now reads +lat_1 only for Winkel Tripel, matching the reader. The summary and objectives describe the same change. The title is 61 characters and 10 words, which fits the single-sentence guidance. It has a conventional fix(map) prefix and does not use vague terms.
Full details: Description check
Explanation
The description follows the template sections: Summary, Testing, UI evidence, and Related issue. The Summary explains the change and its reason. Testing lists the clippy and test commands, the 295 passing tests, and why no new test was added. UI evidence states 'No visible UI change.', which the template allows. The Related issue section is missing. The template placeholder was removed, and the description refers to #1781 only in the Summary text, not as a closing or part-of reference.
Finishing Touches
Generate unit tests (beta)
Commit to this branch
Create a new PR
Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.
A rabbit checks the latitude line, For Winkel Tripel, values must align. Other projections start at zero, The map now follows that rule to a T. Hops of joy across the grid!
Recent review info
Run configuration
Source mode: Auto
Repository: Unavailable
Branch: Unavailable
File: No configuration file from this repository applied
Commit: Unavailable
Layers: Organization UI
Review profile: ASSERTIVE
Plan: Advanced
Run ID: 275dcd2e-ba79-4274-a315-e09b6d0ceb52
Commits
Reviewing files that changed from the base of the PR and between c603c32 and 24df25f.
Files selected for processing: 1
crates/map/src/projection.rs
Included review availability: This review used your included allowance. Your plan provides up to 8 included reviews per hour; 7 remain after this review.
Reviewed the one-file change to crates/map/src/projection.rs. The change makes Projector::new read +lat_1 only for Winkel Tripel, so Natural Earth and Eckert IV now ignore it, as WorldProjection::from_proj4 in crates/data/src/raster/crs.rs already does. I found nothing worth an inline comment.
Bugs: None. The match guard compiles against the in-scope kind, which is Kind with PartialEq. Winkel Tripel behaviour is unchanged. For the other world kinds, unit_forward ignores cos_phi1, and World::lat1 is only read by the Winkel Tripel arm, so the 0.0 default is harmless. Confidence: high.
Security: None.
Performance: None.
Quality: Nit, optional. The guard arm (Some(v) if kind == …) is a different shape from the reader's .filter(|_| kind == W::WinkelTripel). Both are correct. Left for the release quality pass. Confidence: high.
AGENTS.md: No violations. No unwrap, expect, panic or unsafe is added, and the error path still uses ?. The added comment is accurate.
Test coverage: No test is added. This is acceptable because the map's inputs can't reach the branch: lookup rejects +lat_1 for robinson, as the existing test at projection.rs:1017 pins, and the world presets and EPSG/ESRI entries carry no +lat_1. Confidence: medium-high. I could not run cargo in this environment, so the claimed clippy and test results (295 passed) are unverified here.
Earlier review threads: None existed, so none were left alone.
Checked and not raised: The map matches +lat_1= with a leading +, while the reader also accepts a bare lat_1=. This predates the PR and can't be reached from the map's inputs.
No inline comments were posted.
Change size
Compared with main (merge base c603c32 to PR head 24df25f).
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
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.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Summary
Follow-up to #1781, which merged while this last review fix was on its way.
Projector::newchecked+lat_1for every world projection it computes, so it refused a Natural Earth or Eckert IV definition with a+lat_1that the reader (WorldProjection::from_proj4) accepts and ignores, as PROJ does. The map now reads+lat_1for Winkel Tripel only, so the two agree on a definition.Reachability is low today: the map takes a preset, an EPSG code or an ESRI code from its table, and none gives these projections a
+lat_1. Raised by the review of #1781 (thread oncrates/map/src/projection.rs).Testing
cargo clippy -p rustgis-map --all-targets -- -D warningsandcargo test -p rustgis-map --lib(295 passed) on Linux. No new test: no map code reaches the branch.UI evidence
No visible UI change.
Summary by CodeRabbit