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] Do not override libpq defaults #3607

Merged
merged 2 commits into from
Oct 17, 2016

Conversation

strk
Copy link
Contributor

@strk strk commented Oct 15, 2016

@strk strk added For Review Requires Tests! Waiting on the submitter to add unit tests before eligible for merging Needs Backporting labels Oct 15, 2016
@strk strk self-assigned this Oct 15, 2016
@strk
Copy link
Contributor Author

strk commented Oct 15, 2016

not sure how to test this, as the function building the connection string is the same building the whole commandline, maybe they should be split to allow for easier unit testing...

@volaya
Copy link
Contributor

volaya commented Oct 15, 2016

There should be a default. Otjherwise, users will have to manually type host and port everytime, so this change will mean a regression in terms of UX.

The part of the code you added to the eecution method is fine, we can keep that, but let's at least have 5432 and 'localhost' as default values, to make it easier to call the algorithm in the most usual case

@strk
Copy link
Contributor Author

strk commented Oct 15, 2016

Isn't the connection string derived from existing connections ?
For people like me relying on env variables or defaults, the wrong default gets in the way, rather than simplifying.

@strk
Copy link
Contributor Author

strk commented Oct 15, 2016

I've just tried "Import Vector into PostGIS database (new connection)" and I confirm it works fine even if I don't feel any of the Host,Database,User,Password fields (as it then correctly rely on libpq defaults)

@strk
Copy link
Contributor Author

strk commented Oct 15, 2016

Actually, I'd drop the "active_schema" default too..

@strk
Copy link
Contributor Author

strk commented Oct 15, 2016

I would note that the current defaults would mostly be the libpq defaults for most people, so omitting them would not change the experience of anyone with a default PostgreSQL install. The only difference would be not seeing the defaults written in the form.

With latest commit I made it clear that those parameters are optional

@strk strk force-pushed the master_2-processing-libpq-defaults branch from cdd96eb to be3e807 Compare October 15, 2016 08:45
@strk
Copy link
Contributor Author

strk commented Oct 15, 2016

Ok I've refactored the code to not change the UX. I've been thinking that a power user would know what to do better than a non-power user. So we're back to the need for a testcase

@strk strk removed the Requires Tests! Waiting on the submitter to add unit tests before eligible for merging label Oct 15, 2016
@strk
Copy link
Contributor Author

strk commented Oct 15, 2016

Ok @volaya I've now added unit test for the connection string, moving it to its own function.
Env variables are still not being used to fill default parameters

@volaya volaya merged commit 904b62b into qgis:master_2 Oct 17, 2016
@strk strk deleted the master_2-processing-libpq-defaults branch May 15, 2017 16:54
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

2 participants