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

[WIP][server] Clean parameters management #7372

Merged
merged 35 commits into from Jul 23, 2018

Conversation

Projects
None yet
4 participants
@pblottiere
Member

pblottiere commented Jul 5, 2018

Description

This PR improves parameters management in WMS and WFS in order to remove duplicated code.

Moreover, considering that a parameter is now (almost) everywhere a Q_ENUM, several errors due to string comparison are fixed. This way, the WFS GetCapabilities document is much more OGC compliant.

Documentation has to be updated for several classes (WIP).

Checklist

  • 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

@pblottiere pblottiere requested review from elpaso and rldhont Jul 5, 2018

@elpaso

elpaso approved these changes Jul 6, 2018

Good improvements!

const QVariant defaultValue = QVariant( "" ) );
int toInt() const;
QStringList toStringListWithExp( const QString &exp = "\\(([^()]+)\\)" ) const;

This comment has been minimized.

@elpaso

elpaso Jul 6, 2018

Contributor

You may want to use a raw string for readability

This comment has been minimized.

@pblottiere

pblottiere Jul 6, 2018

Member

Thanks for the review @elpaso!

Can you give me an example of what you mean?

This comment has been minimized.

@elpaso

elpaso Jul 6, 2018

Contributor

See this article for an explanation: http://www.informit.com/articles/article.aspx?p=2064649&seqNum=2 , and https://en.cppreference.com/w/cpp/language/string_literal
Basically you don't need to escape the backslashes. But it's really a minor thing: feel free to ignore my comment.

@nyalldawson nyalldawson added the API label Jul 6, 2018

@nyalldawson nyalldawson added this to the 3.4 milestone Jul 6, 2018

@rldhont

rldhont approved these changes Jul 6, 2018

@pblottiere

This comment has been minimized.

Member

pblottiere commented Jul 6, 2018

Hi @nyalldawson,

Travis is currently green, but I don't know why... Actually, some new methods are without documentation, and I was expecting Travis to be red due to the PyQgsDocCoverage test.

Am I missing anything?

Thanks :)

@nyalldawson

This comment has been minimized.

Contributor

nyalldawson commented Jul 6, 2018

@pblottiere Oddly, only certain files are included in the server dox - see https://github.com/qgis/QGIS/blob/master/doc/CMakeLists.txt#L108 . I suspect this should be replaced with a folder based approach like the other qgis libraries use.

@pblottiere pblottiere referenced this pull request Jul 7, 2018

Merged

[server] Update doc path for server #7378

0 of 8 tasks complete
@pblottiere

This comment has been minimized.

Member

pblottiere commented Jul 7, 2018

I suspect this should be replaced with a folder based approach like the other qgis libraries use.

@nyalldawson Thanks for pointing this out, I am taking a look.

@pblottiere pblottiere force-pushed the pblottiere:server_clean branch 2 times, most recently from f9ffb7b to e946576 Jul 13, 2018

@pblottiere

This comment has been minimized.

Member

pblottiere commented Jul 20, 2018

Travis is green.

I'll merge this PR tomorrow if I don't have any comments in the meantime.

@pblottiere pblottiere force-pushed the pblottiere:server_clean branch from b2e25b8 to e48de20 Jul 23, 2018

@pblottiere pblottiere merged commit cc1511c into qgis:master Jul 23, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment