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 test #7

Merged
merged 6 commits into from
Jan 2, 2020
Merged

Refactor test #7

merged 6 commits into from
Jan 2, 2020

Conversation

javierggt
Copy link
Contributor

  • Refactor code to move tests into a submodule.
  • Pep8/flake8

@javierggt
Copy link
Contributor Author

This PR is only two commits. It is easier to see the changes separately, because the second one is pep8/flake8.

@javierggt
Copy link
Contributor Author

javierggt commented Dec 26, 2019

NOTE: I don't mind merging before/after the scm_version PR (#6). They are conflicting changes, but they are simple changes, so I can do it either way.

@taldcroft
Copy link
Member

This was not passing tests locally because _norm was not included in the top-level from quatutil import *. So I just made that function public by changing _norm() to norm() .

Also, the setup.py was not resulting in a working (installable) distribution. I believe one needs to either call find_packages() or explicitly provide them in packages. What is confusing to me is that I looked at the source distribution tarball generated with 0d3912c and with my version dc0317a, and by eye I could not see an obvious difference. But only the latter will import after pip install . into a test env.

What we really should be doing is just deprecating this package completely and moving remaining bits of functionality into chandra_aca.transform. I'm opening an issue FWIW.

@taldcroft
Copy link
Member

About removing this package, I just checked and it does get used in a lot of places. Probably not worth the effort, though we could port all functionality to chandra_aca.transform and recommend that for new code.

@taldcroft
Copy link
Member

I have reviewed this and tested and it all looks good to me now, so merging.

@taldcroft taldcroft merged commit 063fdcc into master Jan 2, 2020
@taldcroft taldcroft deleted the refactor_test branch January 2, 2020 16:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants