Skip to content

removed unnecessary helper functions from io.ascii test file#16908

Merged
mhvk merged 6 commits into
astropy:mainfrom
uorae:main
Aug 31, 2024
Merged

removed unnecessary helper functions from io.ascii test file#16908
mhvk merged 6 commits into
astropy:mainfrom
uorae:main

Conversation

@uorae

@uorae uorae commented Aug 30, 2024 •
edited
Loading

Copy link
Copy Markdown
Contributor

Description

This pull request is to address the needless functions in a helper file for io.ascii.

Fixes #15782

  • assert_equal, assert_true and assert_almost_equal are removed from common.py
  • all instances in test files are changed
  • By checking this box, the PR author has requested that maintainers do NOT use the "Squash and Merge" button. Maintainers should respect this when possible; however, the final decision is at the discretion of the maintainer that merges the PR.

@github-actions

github-actions Bot commented Aug 30, 2024

Copy link
Copy Markdown
Contributor

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.

  • Do the proposed changes actually accomplish desired goals?
  • Do the proposed changes follow the Astropy coding guidelines?
  • Are tests added/updated as required? If so, do they follow the Astropy testing guidelines?
  • Are docs added/updated as required? If so, do they follow the Astropy documentation guidelines?
  • Is rebase and/or squash necessary? If so, please provide the author with appropriate instructions. Also see instructions for rebase and squash.
  • Did the CI pass? If no, are the failures related? If you need to run daily and weekly cron jobs as part of the PR, please apply the "Extra CI" label. Codestyle issues can be fixed by the bot.
  • Is a change log needed? If yes, did the change log check pass? If no, add the "no-changelog-entry-needed" label. If this is a manual backport, use the "skip-changelog-checks" label unless special changelog handling is necessary.
  • Is this a big PR that makes a "What's new?" entry worthwhile and if so, is (1) a "what's new" entry included in this PR and (2) the "whatsnew-needed" label applied?
  • At the time of adding the milestone, if the milestone set requires a backport to release branch(es), apply the appropriate "backport-X.Y.x" label(s) before merge.

github-actions[bot]
github-actions Bot reviewed Aug 30, 2024
View reviewed changes

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown
Contributor

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.

@github-actions

This comment was marked as outdated.

Sign in to view
@uorae uorae closed this Aug 30, 2024
@uorae uorae reopened this Aug 30, 2024
@uorae uorae marked this pull request as ready for review August 30, 2024 08:39
@pllim pllim added this to the v7.0.0 milestone Aug 30, 2024
@pllim pllim requested a review from eerovaher August 30, 2024 13:43
pllim
pllim reviewed Aug 30, 2024
View reviewed changes

@pllim pllim left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thank you!

Is there a reason why you requested maintainer to not use "Squash and Merge"? If you do not allow that option, then please drop 760a0fa instead of adding a new commit to delete the change log.

Comment thread docs/changes/io.ascii/16908.bugfix.rst Outdated
@pllim

pllim commented Aug 30, 2024

Copy link
Copy Markdown
Member

pre-commit.ci autofix

eerovaher
eerovaher approved these changes Aug 30, 2024
View reviewed changes

@eerovaher eerovaher left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This reduces the amount of code used for running io.ascii tests without making the tests any less informative. It should be good to merge if the commits are squashed, but the sub-package maintainers should do the final review.

mhvk
mhvk approved these changes Aug 31, 2024
View reviewed changes

@mhvk mhvk left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Nice! Agreed that the commits should be squashed (or a rebase done making a small set of logical commits).

@mhvk

mhvk commented Aug 31, 2024

Copy link
Copy Markdown
Contributor

I see that the don't squash box is now unticked, so I'll go ahead and merge - I think this is an obvious improvement where I doubt anyone will mind not having to do further review. Thanks, @uorae!

@mhvk mhvk merged commit 29e2652 into astropy:main Aug 31, 2024
@bsipocz

bsipocz commented Sep 3, 2024

Copy link
Copy Markdown
Member

A deprecation as a heads-up would have been nice here as this function was used downstream (and astroquery might not be the only one doing so).

@bsipocz bsipocz mentioned this pull request Sep 3, 2024
Merged
@bsipocz

bsipocz commented Nov 11, 2024

Copy link
Copy Markdown
Member

I run into this again as now @pllim was complaining about that astroquery needs to have a release, practically asap.

These explicitly part of the public API functions just simply should not have been removed without a deprecation. The deprecation pattern and policy in place for a reason, and it should have come up during the review process. It's totally not nice to forcefully make downstream to release for reasons such as this removal.

@pllim

pllim commented Nov 11, 2024

Copy link
Copy Markdown
Member

Well, if it only affects testing, maybe not as urgent, unless you have users who also run similar testing regularly. It was just a FYI issue. I reused the same text for other packages. Sorry for the confusion!

@bsipocz

bsipocz commented Nov 11, 2024

Copy link
Copy Markdown
Member

The "needs a new release" is not a FYI phrasing, especially if it is due to a mistake upstream has made. We do dev testing, and do RC testing and smoke out all the issues in the dev cycle, so the flipside of that would be for upstream to actually follow their own deprecation policies.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@pllim pllim pllim left review comments

@github-actions github-actions[bot] github-actions[bot] left review comments

@mhvk mhvk mhvk approved these changes

@eerovaher eerovaher eerovaher approved these changes

@taldcroft taldcroft Awaiting requested review from taldcroft taldcroft is a code owner

@dhomeier dhomeier Awaiting requested review from dhomeier dhomeier is a code owner

Assignees

No one assigned

Labels

io.ascii no-changelog-entry-needed Refactoring testing

Projects

None yet

Milestone

v7.0.0

Development

Successfully merging this pull request may close these issues.

Remove needless functions from an io.ascii test helper file

5 participants

@uorae @pllim @mhvk @bsipocz @eerovaher