-
Notifications
You must be signed in to change notification settings - Fork 361
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
WKTParser extension support #3244
WKTParser extension support #3244
Conversation
a5a3b18
to
3e29f41
Compare
3e29f41
to
c7181ea
Compare
} | ||
|
||
case class ExtensionProj4(value: String) extends Extension { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm still wondering was it a good idea to make it a part of the parsing DSL or not.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you describe why or why not? I think I'm lacking some project context to know more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm also wondering, mb there is a better name for Extention that is just anything and not Proj4?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Including as part of the DSL seems fine, its what we do for all the other things we need to parse, so that feels consistent at least.
IDK about better names. All the other parsers match the names being parsed out, like Parser[GeocCS]
looks for GEOCCS[]
. So what's here is consistent with the current naming convention. Kinda annoying that its such a generic word that could be confused in other contexts, but it seems pretty well isolated here at first glance, so 🤷
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Core functionality looks ok, I just posed a few questions in order to better orient myself for this change.
40f11e9
to
f5efae9
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🎉
Overview
Checklist
Closes #3241