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

Add parser for OGC Filter version 1.0 and 1.1 #773

Closed
wants to merge 1 commit into from
Closed

Add parser for OGC Filter version 1.0 and 1.1 #773

wants to merge 1 commit into from

Conversation

bartvde
Copy link
Member

@bartvde bartvde commented Jun 10, 2013

Adds a parser for OGC Filter Encoding version 1.0.0 and 1.1.0. The parser
supports read and write.

This is not yet ready for review since it depends on the javascript filter language that @tschaub is working on, this will need to be integrated later, but I thought I'd put this out here already.

Adds a parser for OGC Filter Encoding version 1.0.0 and 1.1.0. The parser
supports read and write.
@twpayne
Copy link
Contributor

twpayne commented Jun 10, 2013

Thanks @bartvde. As a polite request, is it possible to split large changes like this into multiple commits? This would make it much easier to review.

@bartvde
Copy link
Member Author

bartvde commented Jun 10, 2013

@twpayne there isn't actually so much code here, it's only 3 classes and they are inter-related. Most of it is XML files for the tests.
Re-reading your comment, you're talking about the amount of commits in the PR? I actually squashed them since I was told by @elemoine this is the way to go for ol3, unless I misinterpreted?

@bartvde
Copy link
Member Author

bartvde commented Jun 10, 2013

Maybe I misinterpreted on the need for squashing, maybe @elemoine only meant following the commit message standard would have been nice, not the squashing part of my previous comment:

#205 (comment)

@twpayne
Copy link
Contributor

twpayne commented Jun 10, 2013

I find that the commit history is a useful narrative for understanding the approach. For example, in this case, it could be something like:

  • Update GML parsers to use Arrays as extents
  • Make ol.parser.ogc.GML#createGeometry public
  • Add ol.parser.XML.prototype.createDocumentFragment
  • Add ol.filter.Comparison
  • Add ol.filter.FeatureId
  • Add ol.filter.Spatial
  • Add ol.parser.ogc.Filter_v1
  • Add ol.parser.ogc.Filter_v1_0_0

This, to me, is much more descriptive and indicates that the commit includes other changes.

@bartvde
Copy link
Member Author

bartvde commented Jun 10, 2013

but still all this narrative could be put into the commit message (I should have done a better job at the commit message in this case, but I did not put too much effort in it since likely a bigger part of this is still gonna change).

@twpayne
Copy link
Contributor

twpayne commented Jun 10, 2013

True, but when looking at the commit history you don't always see the full commit message, and single large commits that affect many different parts of the codebase don't play well with git bisect.

@tschaub
Copy link
Member

tschaub commented Jun 12, 2013

it depends on the javascript filter language that @tschaub is working on

Fortunately, it's a language we all should be familiar with :)

We can work on this in either direction. I'm close to having a pull request ready for expression parsing. It will replace our current ol.Expression and ol.filter.* classes. For example, an ol.expression.Comparison will replace ol.filter.Comparison. And the ol.expression.parse function is used to generate expressions from strings. You can get an idea for what it will look like in the tests. E.g. parsing relational operators or equality operators will produce ol.expression.Comparison, parsing binary logical operators produces ol.expression.Logical, etc. Instead of things like ol.filter.BBOX, we'll have ol.expression.Call with well-known functions (that we provide). These expressions are parsed from strings (a subset of ES-5) and can be serialized again as OGC Expressions, Filter Encoding, etc. where appropriate.

Anyway, I'll try to wrap up a pull request today. Then we can talk about where to do the integration work.

@bartvde
Copy link
Member Author

bartvde commented Jun 13, 2013

@tschaub let me know when your PR request is ready, and I'll adapt my code gladly to use the new structures.

@elemoine
Copy link
Member

Maybe I misinterpreted on the need for squashing, maybe @elemoine only meant following the commit message standard would have been nice, not the squashing part of my previous comment:

Indeed :)

@bartvde
Copy link
Member Author

bartvde commented Jun 27, 2013

Closing this one, will open up a new one once I finish the integration

@bartvde bartvde closed this Jun 27, 2013
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants