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 get_info_by_id to VOTable interface #3633

Merged
merged 2 commits into from
Mar 27, 2015
Merged

Conversation

mdboom
Copy link
Contributor

@mdboom mdboom commented Mar 27, 2015

Addresses a shortcoming shared on the mailing list thread "getting id value from xml files" by Grigoris Maravelias. http://mail.scipy.org/pipermail/astropy/2015-March/003661.html

There is currently no way to just find an INFO element by its ID (though there is for everything else with an ID). This adds that.

@mdboom mdboom added this to the v1.1.0 milestone Mar 27, 2015
@pllim
Copy link
Member

pllim commented Mar 27, 2015

@mdboom , you already made the changes and this seems not related to Cone Search, so why am I assigned? I am confused.

@mdboom
Copy link
Contributor Author

mdboom commented Mar 27, 2015

In astropy, we are experimenting with a new policy for PR assignment. People assigned to a PR are responsible for reviewing and merging it -- which is usually someone different from the author of the PR. Issue assignment still just means "I'm working on it". So, I assigned this to you as one of the other VOTable experts on the team.

@pllim
Copy link
Member

pllim commented Mar 27, 2015

I can't say I am an "expert", but I can help with review. Although I cannot merge because I do not have write access.

@@ -258,7 +258,7 @@
</TR>
</TABLEDATA>
</DATA>
<INFO ID="Error" name="Error" value="One might expect to find some INFO here, too..."/>
<INFO ID="ErrorInfo" name="Error" value="One might expect to find some INFO here, too..."/>
Copy link
Member

Choose a reason for hiding this comment

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

Why did ID change from Error to ErrorInfo?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This file is what we expect to get out when we load regression.xml, convert it to binary and back and output it in VOTable version 1.3. In the original file, regression.xml, it used to be that only name was specified, so ID was created based on it. In order to test that ID and name are also treated separately, I added an ID to the original that doesn't match name.

@pllim
Copy link
Member

pllim commented Mar 27, 2015

Okay, looks fine to me. Okay to merge.

mdboom added a commit that referenced this pull request Mar 27, 2015
Add get_info_by_id to VOTable interface
@mdboom mdboom merged commit ae8a139 into astropy:master Mar 27, 2015
@embray
Copy link
Member

embray commented Mar 27, 2015

I don't see any reason @pllim doesn't have write access.

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

Successfully merging this pull request may close these issues.

None yet

3 participants