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

Corion/exporter inheritance #155

Merged
merged 3 commits into from
May 10, 2021

Conversation

Corion
Copy link

@Corion Corion commented May 7, 2021

This patch does two things:

  1. Remove the inheritance from Exporter in HTTP::Status

This might break usage like

HTTP::Status->export_to_level(...)
  1. Remove superfluous requirement of Perl 5.002

The use of our requires Perl 5.6 anyway

Max Maischein added 2 commits May 7, 2021 08:41
This might break some arcane usages like

HTTP::Status->import_to_level()

that weren't documented usage anyway
The "our" syntax requires 5.6 anyway
@skaji
Copy link
Member

skaji commented May 7, 2021

@Corion Can you update Changes file?

diff --git a/Changes b/Changes
index db255951..167535a3 100644
--- a/Changes
+++ b/Changes
@@ -1,6 +1,7 @@
 Revision history for HTTP-Message

 {{$NEXT}}
+    - (Your change entry goes here)

 6.29      2021-03-06 04:50:34Z
     - fix issue with HTTP::Request internal cache for canonical url when using

Copy link
Member

@skaji skaji left a comment

Choose a reason for hiding this comment

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

LGTM

@Corion
Copy link
Author

Corion commented May 7, 2021

I've also updated the Changes file with the two changes

Copy link
Member

@genio genio left a comment

Choose a reason for hiding this comment

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

Other than this one style question, it looks good to me! Thanks, @Corion

@@ -5,7 +5,7 @@ use warnings;

our $VERSION = '6.30';

use base 'Exporter';
use Exporter 5.57 'import';
Copy link
Member

Choose a reason for hiding this comment

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

Just a style question here. I know in our other LWP things we tend to:

use Foo::Bar qw(thing_to_import);

https://github.com/libwww-perl/libwww-perl/blob/master/lib/LWP/UserAgent.pm#L16 for example.

Since we already have the version required for Exporter in our build, should this instead be

use Exporter qw(import);

Copy link
Member

Choose a reason for hiding this comment

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

Right, that would be in line with other uses. I can occasionally run https://metacpan.org/pod/perlimports on the repo to enforce style. That will rewrite imports. It will preserve the version number, though.

@oalders
Copy link
Member

oalders commented May 7, 2021

The build test failure is what was reported in #101. There's a fix in that issue that we could apply.

@oalders
Copy link
Member

oalders commented May 7, 2021

This search for export_to_level finds no results on GitHub: https://grep.app/search?q=HTTP%3A%3AStatus-%3Eexport_to_level&filter[lang][0]=Perl

@oalders oalders merged commit c4b608a into libwww-perl:master May 10, 2021
@oalders
Copy link
Member

oalders commented May 10, 2021

New version released, thanks @Corion!

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.

4 participants