-
-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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 needless functions from an io.ascii test helper file #15782 issue sloved #15787
base: main
Are you sure you want to change the base?
Conversation
assert_equal() assert_almost_equal() assert_true()
Thank you for your contribution to Astropy! 🌌 This checklist is meant to remind the package maintainers who will review this pull request of some common things to look for.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Welcome to Astropy 👋 and congratulations on your first pull request! 🎉
A project member will respond to you as soon as possible; in the meantime, please have a look over the Checklist for Contributed Code and make sure you've addressed as many of the questions there as possible.
If you feel that this pull request has not been responded to in a timely manner, please send a message directly to the development mailing list. If the issue is urgent or sensitive in nature (e.g., a security vulnerability) please send an e-mail directly to the private e-mail feedback@astropy.org.
Thanks! However, failures are real. Please do try run the test suite locally and see. https://docs.astropy.org/en/latest/development/testguide.html https://docs.astropy.org/en/latest/development/workflow/development_workflow.html#pre-commit |
i have replaced the functions as mentioned but unable to run the tests |
Two things:
|
Thank you for working on this. 2 general comments:
|
Your last commit brings in a number of changes inside your I suggest you |
@dhomeier i have double checked those functions uses and there are none remaining, the numpy.testing.assert_** is similar to those but i have not touched those |
There are at least 4 imports left, see https://github.com/astropy/astropy/actions/runs/7321280634/job/19941477016#step:10:301 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks; could you try to at least install the pre-commit
setup locally as documented in the second link Pey Lian posted above? I think this should function without needing a working compiler installation, and you can then check those warnings and diagnostics like https://results.pre-commit.ci/run/github/2081289/1703509121.Z6WkqI4KQVm9d2te2s3qhg before committing.
@@ -8,7 +8,6 @@ | |||
|
|||
from .common import ( | |||
assert_almost_equal, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
E.g. this – you may have removed all actual uses, but some imports are left (again, I really wonder why flycheck
did not catch the unused import).
Correcting myself; pre-commit
did fail as well – are you able to run this locally?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Someone familiar with Windows configuration will need to look into your |
The tox errors look the same as for pre-commit, or perhaps tox depends on pre-commit working. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The introduction of irrelevant IDE files is undesirable.
If Windows is giving you too much trouble, please consider using a Linux distro via WSL2. |
I have already requested clearing those from the branch. This will need squashing anyway – do you know if that will take care of removing those from the history for good? |
Re: #15787 (comment) Indeed squashing would clear them out but they have to disappear from the final diff first for that to work. Unless you remember to use the Squash and Merge button, it is safer to squash it locally and force push so we can have a clean PR history before merge. If the files accidentally get in |
Sorry my college started that's why I was unable to work at it |
Hi humans 👋 - this pull request hasn't had any new commits for approximately 4 months. I plan to close this in 30 days if the pull request doesn't have any new commits by then. In lieu of a stalled pull request, please consider closing this and open an issue instead if a reminder is needed to revisit in the future. Maintainers may also choose to add keep-open label to keep this PR open but it is discouraged unless absolutely necessary. If this PR still needs to be reviewed, as an author, you can rebase it to reset the clock. If you believe I commented on this pull request incorrectly, please report this here. |
assert_equal()
assert_almost_equal()
assert_true()
Description
Fixes #15782