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

Field calculator: provide a list of default field types #8990

Merged
merged 2 commits into from Jan 29, 2019

Conversation

elpaso
Copy link
Contributor

@elpaso elpaso commented Jan 25, 2019

in case the provider does not (WFS is one of them).

Rationale: consider that there is not such
a thing like a list of supported types for WFS
and parsing the particular describeFeatureType
for the layer would restrict the types to only
those actually existing in the layer, but
we are dealing with virtual fields here (because
WFS has no column add capabilities) so
let's give the users a minimal set of useful
types to play with.

Fixes #21086

in case the provider does not (WFS is one of them).

Rationale: consider that there is not such
a thing like a list of supported types for WFS
and parsing the particular describeFeatureType
for the layer would restrict the types to only
those actually existing in the layer, but
we are dealing with virtual fields here (because
WFS has no column add capabilities) so
let's give the users a minimal set of useful
types to play with.

Fixes qgis#21086
@nyalldawson
Copy link
Collaborator

Could an alternative solution be to move this default list to wfs provider as its (estimated) native types? It would also fix the issue for the add column actions at the same time then.

@elpaso
Copy link
Contributor Author

elpaso commented Jan 25, 2019

Could an alternative solution be to move this default list to wfs provider as its (estimated) native types?

Yes, it's the alternative solution, just move this piece of code into WFS provider.
I thought about it and I felt that it was advantageous to put the default in the field calc, because it would have covered the case of other (future) providers that do not have a list of types (maybe because like WFS there is no list of standard types).

But I'm totally fine with moving the code in the provider.

It would also fix the issue for the add column actions at the same time then.

What issue? (WFS does not allow to "alter table")

@nyalldawson
Copy link
Collaborator

Ah ok, I didn't realise you couldn't add fields here. (But wouldn't that also affect field calculator?)

In any case, I think this those belong in the wfs provider, with other affected providers also setting their own list of reasonable fallback defaults.

@elpaso
Copy link
Contributor Author

elpaso commented Jan 26, 2019

Ah ok, I didn't realise you couldn't add fields here. (But wouldn't that also affect field calculator?)

We are talking about virtual fields here, it's a legitimate use case even if the data source has no addColumn capabilities.

In any case, I think this those belong in the wfs provider, with other affected providers also setting their own list of reasonable fallback defaults.

AFAIK the WFS is the only one which does not override the default implementation (which is an empty list of field types) , but there is also a possibility to add this default list to the base class: it's a sensible minimal subset.

@elpaso
Copy link
Contributor Author

elpaso commented Jan 26, 2019

@nyalldawson forgot to mention that a reason for NOT adding the defaults to the provider is side effects where the list of fields is used outside of the field calculator (I did not check), by adding the default inside the field calculator class we can be absolutely sure that there are/will-be no other parts of the code affected by seeing a list of field types coming from a data source without addColumn capabilities.

@nyalldawson
Copy link
Collaborator

Hmm, ok. Sorry I missed the original issue here. But then - shouldn't we ALWAYS allow ALL field types for virtual fields?

@elpaso
Copy link
Contributor Author

elpaso commented Jan 27, 2019

Maybe, even if I don't really find a good reason to add all variants of numeric integers and floating point types: there is no storage involved here. This is why I chose the minimalist approach.

@elpaso
Copy link
Contributor Author

elpaso commented Jan 27, 2019

Oh, sorry I missed the "always" part. Yes, I agree that virtual fields should "always" share the same field type set.

@elpaso
Copy link
Contributor Author

elpaso commented Jan 28, 2019

@nyalldawson see my last commit

@elpaso elpaso merged commit d61caab into qgis:master Jan 29, 2019
@elpaso elpaso deleted the bugfix-21086-wfs-field-calculator branch January 29, 2019 11:10
elpaso added a commit to elpaso/QGIS that referenced this pull request Feb 6, 2019
…lator

Field calculator: provide a list of default field types

Cherry-picked from master d61caab
@elpaso elpaso mentioned this pull request Feb 6, 2019
nyalldawson pushed a commit that referenced this pull request Feb 6, 2019
Field calculator: provide a list of default field types

Cherry-picked from master d61caab
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