Fixing single letter variables in vsa,wfau,xmatch

[sebaucillon]

issue #1572 (renaming single letter variables in vsa, wfau, xmatch, tested by @mfonism )
issue #1217 (changing docs from DR1 to DR2, untested)

[codecov]

Codecov Report

Merging #1880 into master will not change coverage.
The diff coverage is 64.28%.

@@           Coverage Diff           @@
##           master    #1880   +/-   ##
=======================================
  Coverage   64.44%   64.44%           
=======================================
  Files         200      200           
  Lines       15962    15962           
=======================================
  Hits        10287    10287           
  Misses       5675     5675           

Impacted Files	Coverage Δ
astroquery/xmatch/core.py	76.82% <60.00%> (ø)
astroquery/wfau/core.py	61.11% <75.00%> (ø)

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 73af9fc...fb37614. Read the comment docs.

[bsipocz]

this seems to be coming from an unrelated commit, please remove the commit

[bsipocz]

OK, now reading your description it's clear that you added changes for two issues in one PR.

The general the suggestion is to avoid this, keep PRs self contained around one topic (either multiple issues of a module when part of a bigger refactor/development, or one issue for multiple modules).

Either way, this change doesn't address the issue. The output in this example should be what the commands above give back, and there are no commented out lines in that output.

[bsipocz]

I would suggest to use descriptive variable names, that is also part of the motivation for this cleanup.
E.g. item --> column or colname
ii -> could either stay or call it count, but it is not better than ii at all.

[bsipocz]

it is not descriptive. Maybe use cat_index

[bsipocz]

use url instead of Uobj

[bsipocz]

There seems to be some misunderstanding about the workflow. You don't need to open PRs in your own fork. You can but it is not necessary as you can and should open a PR to the main astroquery repo from a branch from your fork (I guess the issue was that once you made your branches correctly, you went to github.com/sebaucillon/astroquery rather than github.com/astropy/astroquery).

So, for the PR to astroquery, it should be done from your feature branch rather than from master. Doing so will save you a lot of headaches, as well as will make it much easier to work on multiple PRs parallel, and keeping up with the code changing in the meantime. Here is some documentation about the workflow: https://docs.astropy.org/en/latest/development/workflow/development_workflow.html#workflow and also a worked out example of a pull request: https://docs.astropy.org/en/latest/development/workflow/git_edit_workflow_examples.html#astropy-fix-example

But now, we can still fix up this PR, by squashing all the commits together, and next time you can work from a feature branch. Squashing is very similar to an interactive rebase, so I'll go with a rebase route. Some docs is here: https://docs.astropy.org/en/latest/development/workflow/development_workflow.html#rebase-but-only-if-asked

In summary, first you check whether the main repo is added as a remote:

git remote -v. You should be able to see something like the following (astropy maybe called upstream, and sebaucillon as origin, depending on whether you followed the astropy guide or a different one to set up your local clone. both are fine, but in the example below I assume it's astropy and sebaucillon)

astropy	git@github.com:astropy/astroquery.git (fetch)
astropy	git@github.com:astropy/astroquery.git (push)
sebaucillon	git@github.com:sebaucillon/astroquery (fetch)
sebaucillon	git@github.com:sebaucillon/astroquery (push)

First sync the content
git fetch --all

Make a copy of your branch, just to be on the safe side and have a backup you can easily come back to is something goes wrong.

git branch copy_master_before_rebase

You start an interactive rebase:

git rebase -i astropy/master

This brings up an editor, with 8 commits. Change the first pick to reword, and change the rest to say fixup rather than pick. Comment out the Gaia docs one. It should look something like this:

reword a06df6f3 Rename single letter internal variables, xmatch.
#pick 78d61cdc Try update gaia docs with DR2, no test.
fixup e4c7127e Rename single-letter variables in wfau.
fixup 1438ee8a Rename single letter variable in vsa.
fixup 6557556a Rename single letter variables, xmatch, except units as u.
fixup 4098c586 Rename single letter variables, wfau, except units as u.
fixup 7b618361 Rename single letter variables, vsa, except units as u.
fixup 16c8b6f1 Fix error in wfau w.r.t. single letter variable units u.

Save and exit. A new editor window will open up, edit the commit message there to add wfau and vsa to the end of the message, as all your chanages will end up in one commit. Save and exit. It should success without issues, and with a message Successfully rebased and updated detached HEAD..

Now, push it back to your remote. It has to be a force push as this is changing the history.

git push sebaucillon HEAD:master --force

You should be all set, and the PR will most likely ready to be merged.

[bsipocz]

Thank you @sebaucillon, this all looks good now.

[sebaucillon]

Thank you @sebaucillon, this all looks good now.

Thank you @bsipocz . I was indeed a bit confused with the whole workflow (I actually had just one remote, origin, with the fetch address being the astropy/astroquery and the push address being my sebaucillon/astroquery clone).

Your comments made things a lot more clear for me. Thanks again.