Tap 1.2.1 #1266
Tap 1.2.1 #1266
Conversation
|
Check results are now reported in the status checks at the bottom of this page. |
|
Hello @jcsegovia! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found: There are currently no PEP 8 issues detected in this Pull Request. Cheers! 🍻 Comment last updated at 2019-05-16 15:35:59 UTC |
Codecov Report
@@ Coverage Diff @@
## master #1266 +/- ##
=========================================
- Coverage 64.6% 61.71% -2.9%
=========================================
Files 166 172 +6
Lines 12917 13957 +1040
=========================================
+ Hits 8345 8613 +268
- Misses 4572 5344 +772
Continue to review full report at Codecov.
|
|
@bsipocz there is an error in Travis CI build, but I am not sure it is due the changes we have done:
|
|
@keflavich @bsipocz it seems there is an exception when creating documentation. I do not know how to access to the log file that contains the error. In any case, I think it is not related to gaia nor tap modules.
|
|
@jcsegovia I'll have a look, but I bet @bsipocz knows better. Have you built successfully locally? |
|
@keflavich I do not create Travis builds locally. I have never used Travis before. |
|
@jcsegovia , (tldr) if the error is coming from doc build, you can probably reproduce it by running p.s. I see that you are TAPping into the service's full potential. 😏 |
|
I am able to reproduce the failure locally, so there is a real issue. |
|
The problem is that you commented out the entire configuration block in |
|
@pllim @keflavich thanks! I was able to find the error by executing 'python setup.py build_sphinx' |
|
lgtm. @bsipocz, anything else, or shall we merge today? |
|
Ohh, I'm sorry, I totally lost track of this. Let me do a review tomorrow (but given the size of the PR I suppose I could mostly do nitpicks, etc rather than anything thorough and trust that this works as it suppose to work). |
|
Actually I would like to ping @smoh to ask whether she is interested doing a review, I suppose she has most of the insights of this module besides the authors of it. |
|
On the last master merge commit: Isn't it a bad idea to merge master into this pull request branch and introduce irrelevant changes from MAST/TESS stuff? |
|
@smoh Merging master won't pull in any irrelevant changes because the diff against master is still zero. There is preference in astropy to rebase rather than merge PRs, but it's not a very big deal. |
| >>> full_qualified_table_name = 'user_<your_login_name>.table_test_from_url' | ||
| >>> query = 'select * from ' + full_qualified_table_name | ||
| >>> job = Gaia.launch_job(query=query) | ||
| >>> results = job.get_resultsjob) |
There was a problem hiding this comment.
| >>> results = job.get_resultsjob) | |
| >>> results = job.get_results(job) |
typo: missing (
There was a problem hiding this comment.
I think there are 9 other same typos ;-).
There was a problem hiding this comment.
You are right. This must be fixed.
You're right, in case of conflicting changes it can definitely can mess up the diff. Right now it's not the case as the TESS changes happened on totally disjunct part of the package. |
There was a problem hiding this comment.
Hi @jcsegovia ,
I've only looked at (part of) tap/core.py and some related modules.
General changes I would suggest are:
- Remove unnecessary
__internal_init. - Reduce
__attributeonly to occasions actually needed. Make most attributes plain (public) and use_attributefor things not intended to be exposed. - Normalize methods in camelCases to underscore_names (I think this was mentioned in the original Tap pull request but quite a bit still left).
Other things I found (not part of code added in this PR):
__parseUrlis a utility method with no reference toself.- Similarly
TapConn.url_encodesimply runsurlencodeimported fromastropy.extern.six.moves.urllib.parse.
I find it too messy and overwhelming to mark comments on all individual places on Github and I suppose it will be same to the authors, too. If the authors are open to it, I will make a pull request to their fork or just let authors make the changes.
If the maintainers or authors would rather let this out first, I wouldn't want to hold back.
| datalink_context=datalink_context, | ||
| port=port, | ||
| sslport=sslport) | ||
| self.__connHandler = tap | ||
| # if connectionHandler is set, use it (useful for testing) |
There was a problem hiding this comment.
- This should be checked first since if connhandler is set there's no need to check other keywords and initialize another TapConn.
- Although I imagine no user is actually going to mess around much with this initialization, there is no consistency check between parameters that may be obtained from
urland parameters that can be given directly as keywords nor a requirement for them to be mutually exclusive.
There was a problem hiding this comment.
You are right. If connHandler is set, no need to create connections.
About arguments consistency, again, you are right. I did not include these checks because the code would grow with multiple if's and, as you have said, this is a quite internal initialization.
| @@ -67,34 +91,54 @@ def __init__(self, url=None, host=None, server_context=None, | |||
| """ | |||
| self.__internalInit() | |||
There was a problem hiding this comment.
Remove __internalInit and do initialization directly here.
| datalink_context=datalink_context, | ||
| port=port, | ||
| sslport=sslport) | ||
| self.__connHandler = tap |
There was a problem hiding this comment.
I would suggest make connHandler public and make it not camelCase.
| tsp.parseData(response) | ||
| print("Done.") | ||
| return tsp.get_table() | ||
|
|
||
| def __load_tables(self, only_names=False, include_shared_tables=False, |
There was a problem hiding this comment.
- Move this method to
TapPlus.load_tables. The more complicated method which can handle additional TAP+ keywords is implemented here in the simpler classTap. It seems very unnatural to implement a more complex function in the simpler class, not use it within that class and call it from subclass that extends this class (TapPlus) as_Tap__load_tables(https://github.com/astropy/astroquery/pull/1266/files#diff-59b6077f0dc145f56a2320f3b5b83e6eR813). This is the opposite of the intended usage of__attributewhich is converted to_Class__attributeto ensure__attributeis that set within thatClassnot the base class.
There was a problem hiding this comment.
You are right. __load_tables should be move to TapPlus. Thanks.
|
Thanks you @smoh.
I would prefer to have the cleanup rather than merge this as is. @jcsegovia - let us know otherwise, e.g. if you have a pressing deadline by this has to be deployed. |
|
A couple of more thoughts from tap/core.py:Tap:
|
|
@bsipocz about deadline: no we do not have a deadline. We can work on this changes before merging this PR. |
|
@smoh thanks for your comments. Please, feel free to modify the code if you want. Probably, we are not going to modify the code until next Thursday. About requests, I did not know it was a requirement for astroquery: I did not test it and I am not sure whether we are going to find any problem or not (we use cookies and multipart forms in several places, we need to check that it works with requests). |
|
@jcsegovia requests is a great package, and astroquery would be a total mess without it. It handles all of the heavy lifting for remote queries, including cookies and multipart form data.
...we probably need more developer-directed documentation on these base classes... |
'set_failed' method). Fixed small typo in docs.
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
…positional arg users
|
I plan to open a follow-up PR that resurrects some of the cleanup from esdc-esac-esa-int#1, hopefully in the next couple of days, but feel free to open your own follow-ups as needed. |
New capabilities added:
-Tables sharing.
-Tables editing.
-Tables deleting.
-Tables upload using pytables & jobs results.
-Cross match.
-Data access.
-Datalink access.