Logical filter touch-ups #501

Merged
merged 3 commits into from Apr 9, 2013

Conversation

Projects
None yet
4 participants
Owner

tschaub commented Apr 7, 2013

This corrects the behavior of the logical OR filter, adds logical NOT, and provides factories for all logical filters.

Owner

ahocevar commented Apr 8, 2013

Nice! Please merge.

Owner

elemoine commented Apr 8, 2013

Looks good to me too. I'd have one API-related comment though: elsewhere in the lib we specialize using inheritance, and with this patch we start specializing using factory functions. Relying on factory functions makes more sense to me, but I think we should be consistent.

elemoine referenced this pull request Apr 8, 2013

Closed

Add ol.Map2D #463

Owner

tschaub commented Apr 8, 2013

Thanks for the reviews.

with this patch we start specializing using factory functions

I don't see that inheritance is an option here. The ol.filter.not function, for example, is just a convenience for this:

new ol.filter.Logical([filter], ol.filter.LogicalOperator.NOT);

(And a logical filter still inherits from the base filter.)

So the not factory is just a convenience function.

In a related note, I'd like to change the filter constructors to take options objects. We have 7 years of painful backwards compatibility that demonstrates (to me) that public facing constructors with ordered arguments are a bad idea. I'm not pretending that we didn't learn lessons with OpenLayers 2.

Update: I see now that you probably meant there could be ol.filter.LogicalNot, ol.filter.LogicalAnd, and ol.filter.LogicalOr types (we could even go deeper if we really wanted to - and you have to overlook that not is a binary operator there). I'm used to being able to do a runtime check for a logical filter. Would you suggest we have an abstract logical filter?

Owner

elemoine commented Apr 9, 2013

I don't like deep class hierarchies, and long inheritance chains may affect perfomance. I'm just looking at building a consistent API. Looking at the current API using factory functions for the OR, AND, and NOT logical filters just looked inconsistent to me. Maybe we should use factory functions everywhere it makes sense, for the XYZ sources for example. Please merge with what you think is best. I don't have a strong opinion on this.

Owner

elemoine commented Apr 9, 2013

An alternative would be to have factory functions for every public-facing type. In this way we could even have ol.layer.openstreetmap, which would return an ol.layer.TileLayer configured with an ol.source.OpenStreetMap. It sounds to me that using factories for everything would allow us to build a consistent API, giving us more freedom and avoiding deep class hierarchies. I'm almost tempted to give it a try.

Owner

elemoine commented Apr 9, 2013

See https://github.com/elemoine/ol3/compare/factories for a proof-of-concept. It doesn't compile, but the simple example works in debug mode. If people agree with the approach I can try to go further.

Contributor

twpayne commented Apr 9, 2013

See https://github.com/elemoine/ol3/compare/factories for a proof-of-concept. It doesn't compile, but the simple example works in debug mode. If people agree with the approach I can try to go further.

See also 6cb3622 from last summer.

I think you'll find the conflict between namespaces and factories troublesome in compiled mode, and will eventually have to split create a separate namespaces (e.g. ol for the factory functions/API and OL for internal stuff). But if you really want, go for it.

Owner

elemoine commented Apr 9, 2013

As discussed offline with @twpayne we can certainly avoid naming collisions if the factories aren't shortcuts but just replacements for the constructors. My "factory" branch demonstrates that already, though it would require more changes (to controls, interactions etc.) to fully demonstrate that the approach works.

Owner

tschaub commented Apr 9, 2013

I like the factory discussion and think it should continue on a separate pull request. Out of curiosity, what is an example of the naming collision?

@tschaub tschaub added a commit that referenced this pull request Apr 9, 2013

@tschaub tschaub Merge pull request #501 from tschaub/logical
Logical filter touch-ups
2ecaf2b

@tschaub tschaub merged commit 2ecaf2b into openlayers:master Apr 9, 2013

1 check passed

default The Travis build passed
Details

tschaub deleted the tschaub:logical branch Apr 9, 2013

Contributor

twpayne commented Apr 9, 2013

I like the factory discussion and think it should continue on a separate pull request. Out of curiosity, what is an example of the naming collision?

An example of a naming collision would be if you wanted your ol.Projection factory to be called ol.projection. ol.projection is already a namespace. This is why ol.projection.get is not called ol.projection.

I think this can be made to work in simple cases, but IIRC things do start to get messy with the compiler after a while.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment