Skip to content

Conversation

agroener
Copy link
Contributor

@agroener agroener commented Nov 4, 2017

This PR contains fixes for:

  1. The observed login issue,
  2. Updated dict.keys() issue which prevented tags from being searched for (containing namespaces)
  3. And restored check_all_jobs/delete_all_jobs basic functionality.

There are still numerous issues w/ the basic functionality, but it is less broken than it was before. Little by little I intend to fix everything.

Interesting notes:
-> In python3 dict.keys() does not return a list; rather, it returns a dict_keys() object which yields values when you iterate over it.
-> The CosmoSim jobid is now stored as an attribute in the jobref tag, rather than in its own tag like it was before.

Both of these cases can be thought of as "code rot", since this package has not been updated in a while.

@pep8speaks
Copy link

pep8speaks commented Nov 4, 2017

Hello @agroener! Thanks for updating the PR.

Cheers ! There are no PEP8 issues in this Pull Request. 🍻

Comment last updated on November 04, 2017 at 15:11 Hours UTC

@astropy-bot
Copy link

astropy-bot bot commented Nov 4, 2017

Hi there @agroener 👋 - 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.

@agroener
Copy link
Contributor Author

agroener commented Nov 4, 2017

@keflavich I noticed there is no official maintainer for this service. Mind if I take over the reigns? I would like to be more proactive with CosmoSim.

@bsipocz
Copy link
Member

bsipocz commented Nov 4, 2017

Thanks @agroener! Ideally cosmosim needs to have remote tests, so these regressions would be picked up sooner by CI rather than by our users.

I don't know about policies about "official" maintenance on the module level, for best of my knowledge we only have a handful one where the archive providers supports the astroquery module, too (e.g. esasky, gaia, mast). So by all means, please feel free to open PRs for cosmosim, or the rest of astroquery to fix or extend things.

@bsipocz bsipocz merged commit f87873f into astropy:master Nov 4, 2017
@agroener
Copy link
Contributor Author

agroener commented Nov 4, 2017

My pleasure @bsipocz! I have been attempting to include tests for CosmoSim for a while now. My next PR will finally fix all of the login/logout (storing and not storing passwords in keyring) use cases, and I will be sure it includes tests.

Thanks for the invite to also contribute to the general astroquery package, too. I am definitely happy to contribute to the project if/when I have the time.

@agroener
Copy link
Contributor Author

agroener commented Nov 4, 2017

Also - do you guys generally hit the 'delete branch' button when a PR is merged in? Does it just delete on the remote repo and not on my local repo?

@keflavich
Copy link
Contributor

@agroener yes, it deletes the remote branch, and yes, I do that. I also occasionally delete excess local branches with git branch --merged | grep -v "\*" | xargs -n 1 git branch -d

@agroener agroener deleted the fix-cosmosim-step1 branch November 4, 2017 15:59
@bsipocz
Copy link
Member

bsipocz commented Nov 4, 2017

(and I never delete them, but I don't really have a good reason why not, hardly ever went back to an already merged branch)

@pllim
Copy link
Member

pllim commented Nov 5, 2017

I delete them too (but it is never really deleted forever if you can memorize the hash, or so I heard). It comes down to personal preference. 😄

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.

5 participants