Skip to content

Fix BNF of CIRCLE, POLYGON and BOX #41

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 2 commits into from
Oct 20, 2020
Merged

Fix BNF of CIRCLE, POLYGON and BOX #41

merged 2 commits into from
Oct 20, 2020

Conversation

gmantele
Copy link
Collaborator

The idea is to update the BNF grammar so that reflecting the updated description of BOX (even if deprecated in ADQL-2.1), CIRCLE and POLYGON.

The updated grammar parts are about:

  • the now optional coord. sys. argument
  • the arguments giving a position can now be either pairs of coordinates or point values

Fixes #29

@gmantele
Fix BNF of CIRCLE, POLYGON and BOX
7416a31 
(optional coord. sys. + points vs coordinates)
@gmantele gmantele added the bug Something isn't working label Jun 17, 2020
@gmantele gmantele added this to the 2.1 milestone Jun 17, 2020
@gmantele
Copy link
Collaborator Author

This PR includes the BNF update of the BOX function though it is deprecated.
In function of the decision taken in #29 , this PR may be modified so that reverting the fix on BOX.

msdemlei
msdemlei previously approved these changes Jun 18, 2020
View reviewed changes
Copy link
Contributor

@msdemlei msdemlei left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should do what it's supposed to do and matches the grammar DaCHS has been using for a few year now. Let's merge.

The clarification that coord_sys needs to be a literal comes in a separate PR then?

@gmantele
Copy link
Collaborator Author

I guess we could also use this PR for that. After all, it is mostly related to CIRCLE and POLYGON.
So, you're right, I'll add a commit to apply this clarification in there.

@gmantele
Apply ADQL-2.0 Erratum-3 :
3283c17 
the coordinate system argument of `POINT`, `BOX`,
`CIRCLE` and `POLYGON` MUST be a string literal.
@gmantele
Copy link
Collaborator Author

The Erratum-3 is now applied (see 3283c17).

msdemlei
msdemlei approved these changes Oct 20, 2020
View reviewed changes
Copy link
Contributor

@msdemlei msdemlei left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Given I've had my hands in this, I may not be the most objective reviewer, but if nobody else steps forward: I'm now happy with all of this.

@molinaro-m
Copy link
Member

Considering the roles in play, I can enforce Markus's approval to enable merge or, @Zarquan can do that with more insight w.r.t. myself.

Zarquan
Zarquan approved these changes Oct 20, 2020
View reviewed changes
Copy link
Member

@Zarquan Zarquan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1 all looks good to me.

@gmantele gmantele merged commit 55f131c into ivoa-std:master Oct 20, 2020
@gmantele gmantele deleted the issue-29-bnf-box-circle-polygon branch October 23, 2020 17:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Zarquan Zarquan Zarquan approved these changes

+1 more reviewer

@msdemlei msdemlei msdemlei approved these changes

Reviewers whose approvals may not affect merge requirements
Assignees
No one assigned
Labels
bug Something isn't working
Projects
None yet
Milestone
2.1
Development

Successfully merging this pull request may close these issues.

Fix BNF of BOX, CIRCLE and POLYGON
4 participants
@gmantele @molinaro-m @msdemlei @Zarquan