Skip to content
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

Add no-op operation. It does nothing. #1391

Merged
merged 2 commits into from Apr 2, 2019

Conversation

Projects
None yet
2 participants
@kbevers
Copy link
Member

commented Mar 28, 2019

Closes #1185

@kbevers kbevers added this to the 6.1.0 milestone Mar 28, 2019

@rouault

This comment has been minimized.

Copy link
Member

commented Mar 28, 2019

noop.cpp lacking in the commit

The following line could be upgraded to use noop

../src/iso19111/c_api.cpp:                ctx, projString.empty() ? "+proj=affine" : projString.c_str());

And, a bit harder, projinfo -s EPSG:4326 -t EPSG:4326 -o PROJ currently returns an empty string. I think that the fix would be to return "+proj=noop" at the end of PROJStringFormatter::toString() in io.cpp if the string is empty. And in that case the above test of the empty string in c_api.cpp could be just removed.

@kbevers

This comment has been minimized.

Copy link
Member Author

commented Mar 28, 2019

noop.cpp lacking in the commit

🤦

The following line could be upgraded to use noop

I was looking for that but couldn't locate it in my first attempt. I had hunch you would remind me where it was :-) I will see if I can figure out how to do it the hard way. If not, I'm taking the easy path...

@kbevers kbevers force-pushed the kbevers:noop branch from 15ea406 to 3d670b8 Mar 28, 2019

@kbevers

This comment has been minimized.

Copy link
Member Author

commented Mar 28, 2019

I've added a very naive adoption of noop in iso19111 code. I saw some test failures locally so this probably won't work here either. It's a starting point for further discussion :)

@rouault

This comment has been minimized.

Copy link
Member

commented Mar 28, 2019

I saw some test failures locally so this probably won't work here either

from Travis output, at least the first failures are no real issues. testprojinfo_out just needs to be updated with the new output

@kbevers

This comment has been minimized.

Copy link
Member Author

commented Mar 29, 2019

from Travis output, at least the first failures are no real issues. testprojinfo_out just needs to be updated with the new output

Yeah, it does seem like that. I had a problem locally that I couldn't immediately determine if was from this change or something else. I've updated the test output for testprojinfo.

@kbevers kbevers force-pushed the kbevers:noop branch from ff06631 to 802b93e Mar 29, 2019

@kbevers kbevers force-pushed the kbevers:noop branch from 802b93e to 47dd4db Mar 29, 2019

@kbevers kbevers merged commit b8a6252 into OSGeo:master Apr 2, 2019

3 checks passed

Travis CI - Pull Request Build Passed
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
coverage/coveralls Coverage increased (+0.01%) to 85.338%
Details

@kbevers kbevers modified the milestones: 6.2.0, 6.1.0 Apr 2, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.