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

Refactor get_password with a common method on QueryWithLogin #1202

Merged
merged 1 commit into from
Sep 4, 2018

Conversation

saimn
Copy link
Contributor

@saimn saimn commented Jul 16, 2018

Following #1198.

@astropy-bot
Copy link

astropy-bot bot commented Jul 16, 2018

Hi there @saimn 👋 - thanks for the pull request! I'm just a friendly 🤖 that checks for issues related to the changelog and making sure that this pull request is milestoned and labeled correctly. This is mainly intended for the maintainers, so if you are not a maintainer you can ignore this, and a maintainer will let you know if any action is required on your part 😃.

Everything looks good from my point of view! 👍

If there are any issues with this message, please report them here.

@pep8speaks
Copy link

Hello @saimn! Thanks for submitting the PR.

Line 259:121: E501 line too long (162 > 120 characters)
Line 261:121: E501 line too long (132 > 120 characters)

@codecov
Copy link

codecov bot commented Jul 16, 2018

Codecov Report

Merging #1202 into master will increase coverage by 0.11%.
The diff coverage is 26.08%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1202      +/-   ##
==========================================
+ Coverage   64.25%   64.36%   +0.11%     
==========================================
  Files         144      144              
  Lines       11451    11431      -20     
==========================================
  Hits         7358     7358              
+ Misses       4093     4073      -20
Impacted Files Coverage Δ
astroquery/cosmosim/core.py 9.15% <0%> (-0.02%) ⬇️
astroquery/eso/core.py 32.57% <0%> (+0.52%) ⬆️
astroquery/query.py 57.29% <23.52%> (-3.42%) ⬇️
astroquery/alma/core.py 54.91% <50%> (+0.68%) ⬆️
astroquery/nrao/core.py 66.48% <50%> (+2.3%) ⬆️

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 a516abd...e0906ab. Read the comment docs.

@saimn
Copy link
Contributor Author

saimn commented Sep 3, 2018

ping 🙌

@keflavich keflavich merged commit a0b0f40 into astropy:master Sep 4, 2018
@saimn saimn deleted the get-password branch September 4, 2018 08:02
@keflavich
Copy link
Contributor

We have a failure after the merge:
https://travis-ci.org/astropy/astroquery/jobs/424141415
Apparently there's a dependency conflict on python 3.4? Is this a temporary conda thing?

@saimn
Copy link
Contributor Author

saimn commented Sep 4, 2018

I saw the same error on my other PR, so not related to the merge. It seems related to the recent release of secretstorage 3.1.0 (jaraco/keyring#338 (comment)) but I don't understand the error. It looks like keyring is installed with conda, so probably a dependency issue.

@bsipocz
Copy link
Member

bsipocz commented Sep 5, 2018

Weird, the keyring version installed is quite old for that version.

@bsipocz
Copy link
Member

bsipocz commented Sep 6, 2018

OK, so the failing build is using the same keyring as it used before. Also secretstorage is still around, but as of 3 days ago, it has a new release. I've just restarted the failing build. If it still fails, we may need to pin the secretstorage version to the one everything worked well before.

@bsipocz
Copy link
Member

bsipocz commented Sep 6, 2018

OK, so the issue has been fixed directly in master.

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

4 participants