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

[processing][needs-docs] allow to exclude features without category from GRASS export #9003

Merged
merged 4 commits into from Jan 30, 2019
Merged

[processing][needs-docs] allow to exclude features without category from GRASS export #9003

merged 4 commits into from Jan 30, 2019

Conversation

alexbruy
Copy link
Contributor

@alexbruy alexbruy commented Jan 28, 2019

Description

Ressurects #8419. IMHO this is quite important fix and would be nice to have in both 3.6 and 3.4 LTR.

As per #8419 while exporting GRASS vectors to a simple features format using v.out.ogr now the -c flag is always used, and this is not expected as it can lead to wrong/unexpected results. On other hand, to keep API stable and produce same results as older 3.x version this PR keeps -c flag enabled by default, allowing user to change it.

If we argee that better to disable -c flag by default, I will update all tests accordingly.

@gioman hope you are fine with it.

Checklist

Reviewing is a process done by project maintainers, mostly on a volunteer basis. We try to keep the overhead as small as possible and appreciate if you help us to do so by completing the following items. Feel free to ask in a comment if you have troubles with any of them.

  • Commit messages are descriptive and explain the rationale for changes
  • Commits which fix bugs include fixes #11111 in the commit message next to the description
  • Commits which add new features are tagged with [FEATURE] in the commit message
  • Commits which change the UI or existing user workflows are tagged with [needs-docs] in the commit message and contain sufficient information in the commit message to be documented
  • I have read the QGIS Coding Standards and this PR complies with them
  • This PR passes all existing unit tests (test results will be reported by travis-ci after opening this PR)
  • New unit tests have been added for core changes
  • I have run the scripts/prepare-commit.sh script before each commit

@gioman
Copy link
Contributor

gioman commented Jan 28, 2019

@alexbruy totally fine!

@alexbruy
Copy link
Contributor Author

@nyalldawson, @gioman if you are ok with these changes I will merge it and backport to 3.4 today

@nyalldawson
Copy link
Collaborator

No strong opinions here - if @gioman is happy, I'm happy!

@gioman
Copy link
Contributor

gioman commented Jan 29, 2019

No strong opinions here - if @gioman is happy, I'm happy!

yes, I think that the flag was added without fully understanding the consequences.

@alexbruy
Copy link
Contributor Author

@gioman so using it by default is ok for you (this way algorithms behavior will be the same as in previous versions) or better to turn it off by default?

@gioman
Copy link
Contributor

gioman commented Jan 29, 2019

@alexbruy the "-c" flag must not be used by default when exporting vectors from GRASS/Processing. It should eventually be possible to enable it (this is what my PR did, and I think also yours). In QGIS 2 this flag was not used (and had no option to enable it, as far as I remember).

@alexbruy
Copy link
Contributor Author

@gioman I know. But in previous QGIS 3.x releases it was used and probably some users now rely on this wrong behavior. I'm not sure if we should break it now. Personally I'm fine with disabling -c flag by default and updating all related test.

@gioman
Copy link
Contributor

gioman commented Jan 29, 2019

But in previous QGIS 3.x releases it was used

to me it was added by mistake, not thinking about the consequences

probably some users now rely on this wrong behavior

I doubt, in my opinion this option leads to a wrong output most of the times.

@alexbruy alexbruy merged commit 662af5c into qgis:master Jan 30, 2019
@alexbruy alexbruy deleted the grass-vector-export branch January 30, 2019 12:35
@alexbruy alexbruy mentioned this pull request Jan 31, 2019
8 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants