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 unused prerequisite for IO::String #1613

Merged
merged 1 commit into from
Oct 29, 2024

Conversation

robrwo
Copy link
Contributor

@robrwo robrwo commented Feb 2, 2024

Also note that IO::String is only necessaryfor Perls before 5.8. Since Rex requires 5.12, this is unnecessary.

@robrwo robrwo marked this pull request as draft February 2, 2024 09:45
@ferki
Copy link
Member

ferki commented Oct 24, 2024

Thanks for contributing, and for your patience, @robrwo, it looks like a good find!

It apparently dates back to the initial commit of the originally out-of-core Rex::Augeas module – and seems already unused even there.

At first glance, I agree we should remove it if it’s unused.

About our contributing guide

This PR does not follow our Contributing guide without explaining why, forcing me to fall back to guessing. Some things I kept wondering about:

  • work status: it’s still marked as draft suggesting “still work in progress”
  • intended goal/scope: I haven’t found a related open issue this would resolve
  • progress: the PR template seems ignored/deleted, giving some extra work to track the checklist
  • safety: no tests covering the related functionality (though I have recently covered most of this module with tests separately, see Add initial Augeas tests #1623)
  • nitpick: the commit message has a typo (missing space)

Towards merging

Nevertheless, let’s see how to move it forward. My ideas:

  • Now that I merged initial Augeas tests, I suggest rebasing this draft PR on top of the current default branch, which also gives an opportunity to fix the typo.
  • Updating the PR should trigger running the test suite again – when tests pass, please remove its draft status to signal “ready to review”.

Do we know a generic solution for unused dependencies?

As a related topic, I wonder how to test for unused modules as a generic approach 🤔

Is there a good tool, or perhaps a Perl::Critic policy which could have caught this unused dependency earlier?

If we can identify a good approach we can turn it into a test, I’d love to discover and discuss that separately.

Also note that IO::String is only necessary for Perls before 5.8. Since
Rex requires 5.12, this is unnecessary.
@ferki ferki force-pushed the rrwo/remove-IO-String branch from 949ff19 to 980ee44 Compare October 28, 2024 16:21
@ferki
Copy link
Member

ferki commented Oct 28, 2024

Given all circumstances here, I decided to rebase this PR on top of current main branch, fix the conflict and the typo, and mark it as ready for review.

I'm also restoring the usual PR checklist here:

  • based on top of latest source code
  • changelog entry included
  • automated tests pass
  • git history is clean
  • git commit messages are well-written

Thanks again for contributing! 🚀

@ferki ferki marked this pull request as ready for review October 28, 2024 16:33
@ferki ferki merged commit 66581aa into RexOps:master Oct 29, 2024
48 checks passed
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