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

Fix incorrect delimator ";" used to separate proj_info().searchpath entries #1497

Merged
merged 1 commit into from
Jun 4, 2019

Conversation

nyalldawson
Copy link
Contributor

... on non-windows platforms, should be ":", not ";"

@nyalldawson
Copy link
Contributor Author

Refs qgis/QGIS#30048

src/4D_api.cpp Outdated Show resolved Hide resolved
entries on non-windows platforms, should be ":"
@rouault
Copy link
Member

rouault commented Jun 3, 2019

@kbevers Do you think we should change the behaviour or keep as it ? After all, the implementation and documentation are consistent currently.
It is true that if we do the analogy with PATH, semi-colon for Windows and colon for Unix are more common.
When looking at the code, I was confused by the fact that path_append() has a #ifdef WIN32 / #else to have a different separator, but it isn't actually used

@kbevers
Copy link
Member

kbevers commented Jun 3, 2019

Do you think we should change the behaviour or keep as it ? After all, the implementation and documentation are consistent currently.
It is true that if we do the analogy with PATH, semi-colon for Windows and colon for Unix are more common.

I think we should accept this PR. As you say, it conforms to normal practice. I believe this is also how we deal with delimeters when parsing PROJ_LIB, right?

When looking at the code, I was confused by the fact that path_append() has a #ifdef WIN32 / #else to have a different separator, but it isn't actually used

That is clearly a bug - delim should have been used in the first place.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants