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

Tickets from gitlab #12

Merged
merged 3 commits into from
Apr 6, 2021
Merged

Tickets from gitlab #12

merged 3 commits into from
Apr 6, 2021

Conversation

jesusjuansalgado
Copy link
Collaborator

No description provided.

@jesusjuansalgado jesusjuansalgado requested a review from jd-au March 29, 2021 14:06
ObjVisSAP.tex Outdated
%%%%%%%%%%%%%%%%%%%% starts here %%%%%%%%%%%%%%%%%%%%
\begin{table}[h]
\centering
\begin{tabular}{|l|l|}
\hline
\begin{lstlisting}[language=SQL]
http://xmmvischeck.esac.esa.int:8080/objvissap/query?
POS=10.68,41.27&T_MAX=59522
POS=10.68,41.27&T=59522/59532
Copy link
Member

Choose a reason for hiding this comment

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

The T= here should be TIME=

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

You are fully right. Thanks!
Corrected

@jesusjuansalgado jesusjuansalgado requested a review from jd-au March 30, 2021 15:54
@jesusjuansalgado
Copy link
Collaborator Author

@jd-au I corrected the typo you found. Could you please take a look to see if everything is OK to do the merging? Thanks!

<PARAM name="INPUT:TIME"
utype="Char.TimeAxis.Coverage.Bounds.Limits"
datatype="float"
unit="d" value="58171.45833/58321.958333">
Copy link
Member

Choose a reason for hiding this comment

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

Should this have xtype="interval"

Copy link
Collaborator Author

@jesusjuansalgado jesusjuansalgado Apr 6, 2021

Choose a reason for hiding this comment

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

The ObjVisSAP protocol does not have any xtype defined for any of its fields. I think this is because it is more based on previous S * AP protocols where xtype was not used (SIAP 1.0, SSAP, SLAP, etc)
I think xtype appeared during the TAP discussions (to allow the description of ADQL types) and it was only incorporated in a S * AP for SIAP 2.0

Most probably, only this field is affected but I have raised a different issue
#13
for a more complete review

If you agree, I would merge this one and deal the new issue in a different pull request after discussion with the rest of the authors

Copy link
Member

Choose a reason for hiding this comment

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

Thanks, that sounds like a good approach.

@jesusjuansalgado jesusjuansalgado requested a review from jd-au April 6, 2021 09:24
@jesusjuansalgado jesusjuansalgado merged commit bcb8eb2 into master Apr 6, 2021
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.

2 participants