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.
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Mapping of additional methods for SpatialRef. #406
Mapping of additional methods for SpatialRef. #406
Changes from 5 commits
082e1bc
9810fb0
f0b7288
05075fc
78a95c2
a407af2
64e1667
e3c111a
e0de31f
db004e9
db77248
adf98cc
e42080a
96ae168
eb80ceb
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So ideally, this would return something like
Result<Option<f64>>
and the user could handle the missing case.But since we can't do that, should we take the default as an argument?Just noticed the other method, should we drop this one and rename?It's currently ambiguous. Are the parameters never 0 in practice?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Behavior of
OSRGetProjParm
is quite tricky, a least from Rust point of view.In C and C++ if value is found it is returned normally, but if value is not found, a value from default parameter is returned, but also error code (if pointer is not null) is set to
OGRERR_FAILURE
. However looking from Rust point of view it may mean not found (therefore is candidate forOption::None
) or can mean any other failure.You can see this in GDAL source code for
OSRGetProjParm
.To sum up:
That's why I used only
Result<f64>
as return type and I also created alsoget_proj_param_or_default
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I'm inclined towards something like a single
pub fn get_proj_param(&self, name: &str) -> Result<Option<f64>>
, which returnsErr
on a string conversion error (basically never), andOk(None)
onOGRERR_FAILURE
. It's not that awkward to use, and should be pretty clear.Both error cases in the implementation are similar, and boil down to "there's no parameter with that name on the
PROJCS
".EDIT: removed the
default
param.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, this is all unfortunate legacy API leakage. I don't know what we should do, or if there are other examples out there (e.g. proj?) but what if we had a new error type defined that indicates
NotSet
? Then, you clould go back to returningResult<f64>
and users who care about a default can match onErr(NotSet)
to handle it?