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

Restructure astroquery #73

Merged
merged 19 commits into from
Apr 15, 2013
Merged

Restructure astroquery #73

merged 19 commits into from
Apr 15, 2013

Conversation

cdeil
Copy link
Member

@cdeil cdeil commented Apr 14, 2013

I propose to restructure the astroquery files like this:

  • astroquery/service/service.py is renamed to astroquery/service/core.py
  • Tests go in astroquery/service/tests/, not all in astroquery/tests.
  • Imports are all relative, even from tests (like in astropy)
  • Names are not (partially) repeated, e.g. astroquery/simbad/sim_parameters.py is renamed astroquery/simbad/parameters.py.

@keflavich @astrofrog What do you think?

I can do it now, but I don't know how rebasing will go for existing PRs. Do you want to merge e.g. #68 and #70 now and then we just keep working on them in new PRs after the restructure?

@ghost ghost assigned cdeil Apr 14, 2013
@keflavich
Copy link
Contributor

I agree with all points laid out here. Perhaps merging those big PRs is not a bad idea; we'll just have to open new issues soon but if you're going to go ahead with the refactor today, I'd be inclined to let you do it against a cleanly merged master.

keflavich added a commit that referenced this pull request Apr 14, 2013
Python 3 and docstring fixes.  Merged as per #73 - we'll split off python3 and sphinx fixes into 2 separate PRs for future work
@cdeil
Copy link
Member Author

cdeil commented Apr 14, 2013

@keflavich @astrofrog Someone please review.

@cdeil cdeil mentioned this pull request Apr 14, 2013
@@ -0,0 +1,19 @@
# Licensed under a 3-clause BSD style license - see LICENSE.rst
Copy link
Contributor

Choose a reason for hiding this comment

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

Any idea why this file is all green? I thought we had it already.

Copy link
Member Author

Choose a reason for hiding this comment

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

But I moved it from astroquery/tests/test_fermi.py to astroquery/fermi/tests/test_fermi.py.
Apparently I changed the content in the same commit when I added the license line and now github shows this as a deletion and addition instead of a move.
Not nice, but I suggest to leave it like that ... I'll be more careful not to move and change content in the same commit in the future.

@keflavich
Copy link
Contributor

I didn't see any serious issues, but with so many small changes I'm not sure I would have caught any. Note that all the travis tests are failing, though, even some that were passing before, e.g. https://travis-ci.org/astropy/astroquery/jobs/6338243. I'm seeing gobs of errors with irsadust, which I suspect are caused by the refactor in some way.

The only non-irsadust issue is this:
E AttributeError: 'MaskedArray' object has no attribute 'to_table' in SimbadResult... investigating.

@@ -1,7 +1,6 @@
Copy link
Contributor

Choose a reason for hiding this comment

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

It might be best to find a different name for this module - maybe leave it as sim_votable.py since we use astropy.io.votable as votable - this may lead to confusion

Copy link
Member Author

Choose a reason for hiding this comment

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

Done.

@cdeil
Copy link
Member Author

cdeil commented Apr 15, 2013

I haven't figured out the causes for the test failures until now ... if you have a tip that would be great.
I don't have much time, but I'll try some more.

@cdeil
Copy link
Member Author

cdeil commented Apr 15, 2013

I fixed the irsa_dust test errors. I forgot to rename tests/t to tests/data in one place.

The remaining simbad test error is not new, I believe. This test just wasn't executed before, because the test function name didn't start with test. Here is the last green travis test run and you can see that the simbad tests aren't run.

My suggestion would be to merge this now and I'll open a new issue for that problem?

This was referenced Apr 15, 2013
@cdeil
Copy link
Member Author

cdeil commented Apr 15, 2013

The Python 3 tests on travis-ci also fail due to #80 .
@keflavich Since travis-ci tests for master are red at the moment anyways, are you OK with merging this now and then fixing the issues one by one in the next days?

@keflavich
Copy link
Contributor

Yes, that sounds reasonable. Merge underway.

keflavich added a commit that referenced this pull request Apr 15, 2013
@keflavich keflavich merged commit 4fae089 into astropy:master Apr 15, 2013
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.

2 participants