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

Remove unnecessary imports update thermal module tests #183

Merged
merged 2 commits into from
Jan 31, 2025

Conversation

samaloney
Copy link
Collaborator

@samaloney samaloney commented Jan 30, 2025

  • Remove imports
  • Updated test to use code under correct module

@KriSun95
Copy link
Collaborator

Would it be appropriate to also remove the import lines in sunkit-spex/sunkit_spex/legacy/fitting/init.py (the imports causing the error Iain showed) in this PR?

I'm more than happy to fork your (@samaloney) bugfix-imports-thermal-tests branch to do this and contribute here but I want to check if you would rather the other import issue to be solved separately.

@samaloney
Copy link
Collaborator Author

samaloney commented Jan 30, 2025

@KriSun95 sounds good but you should now be able to modify this PR without making another fork

git remote add samaloney https://github.com/samaloney/sunkit-spex.git
git fetch samaloney
git checkout bugfix-imports-thermal-tests

do stuff

git add -u
git commit
git push samaloney bugfix-imports-thermal-tests

@KriSun95
Copy link
Collaborator

Fantastic! I will do this shortly when I get the chance. Thanks for taking the time to show the commands I need as well!

@KriSun95
Copy link
Collaborator

I also ran tox to build the documentation and the example gallery seems OK (despite the fact I need to go in and fix the NuSTAR one) so removing the imports doesn't seem to have broken anything.

@samaloney samaloney added the No Changelog Entry Needed Skip all changelog checks. label Jan 31, 2025
@samaloney samaloney requested review from DanRyanIrish, KriSun95 and jajmitchell and removed request for DanRyanIrish January 31, 2025 14:27
Copy link
Collaborator

@KriSun95 KriSun95 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Copy link
Contributor

@jajmitchell jajmitchell left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tested the import, all looks good to me.

@settwi settwi merged commit 2125410 into sunpy:main Jan 31, 2025
14 checks passed
@samaloney samaloney deleted the bugfix-imports-thermal-tests branch January 31, 2025 22:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
No Changelog Entry Needed Skip all changelog checks.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants