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
Sys::Hostname::hostname warn on spurious arguments #14662
Comments
From @epaCreated by @epaSys::Hostname::hostname takes no arguments. But it silently ignores I found calls to hostname('some-host'), probably a mistaken attempt to hostname() should issue a warning if arguments are provided, since they Perl Info
|
From zefram@fysh.orgEd Avis wrote:
It should throw an exception. -zefram |
The RT System itself - Status changed from 'new' to 'open' |
From @epaI agree, it should throw an exception. I proposed the more timid alternative of just warning because I know that if it started throwing exceptions, existing and "working" code would break. If the p5-porters are happy to take the bold approach of just dying on any argument passed to hostname(), I am all for it. -- ______________________________________________________________________ |
From @rjbsOn Mon Apr 20 03:46:25 2015, eda@waniasset.com wrote:
I'd be okay with just making it fatal, but I don't think it's in line with policy, and I don't think it's worth an exception. Care to provide a patch that issues a deprecation-category warning on args to hostname? -- |
From @tonycozOn Tue May 26 05:08:09 2015, rjbs wrote:
How about the attached? Tony |
From @tonycoz0001-perl-124349-deprecation-warning-on-hostname-with-arg.patchFrom dfb56efaaf66c156f4025ae1f0bc34b00596d441 Mon Sep 17 00:00:00 2001
From: Tony Cook <tony@develop-help.com>
Date: Tue, 17 Nov 2015 11:56:12 +1100
Subject: [perl #124349] deprecation warning on hostname() with arguments
---
ext/Sys-Hostname/Hostname.pm | 5 ++++-
ext/Sys-Hostname/t/Hostname.t | 32 +++++++++++++++++++++++---------
2 files changed, 27 insertions(+), 10 deletions(-)
diff --git a/ext/Sys-Hostname/Hostname.pm b/ext/Sys-Hostname/Hostname.pm
index 42e9293..d53410e 100644
--- a/ext/Sys-Hostname/Hostname.pm
+++ b/ext/Sys-Hostname/Hostname.pm
@@ -11,10 +11,12 @@ our @EXPORT = qw/ hostname /;
our $VERSION;
+use warnings ();
+
our $host;
BEGIN {
- $VERSION = '1.20';
+ $VERSION = '1.21';
{
local $SIG{__DIE__};
eval {
@@ -27,6 +29,7 @@ BEGIN {
sub hostname {
+ @_ and warnings::warnif("deprecated", "hostname() doesn't accept any arguments");
# method 1 - we already know it
return $host if defined $host;
diff --git a/ext/Sys-Hostname/t/Hostname.t b/ext/Sys-Hostname/t/Hostname.t
index 40352ba..a8c259d 100644
--- a/ext/Sys-Hostname/t/Hostname.t
+++ b/ext/Sys-Hostname/t/Hostname.t
@@ -10,14 +10,28 @@ BEGIN {
use Sys::Hostname;
-eval {
- $host = hostname;
-};
+use Test::More tests => 4;
-if ($@) {
- print "1..0\n" if $@ =~ /Cannot get host name/;
-} else {
- print "1..1\n";
- print "# \$host = '$host'\n";
- print "ok 1\n";
+SKIP:
+{
+ eval {
+ $host = hostname;
+ };
+ skip "No hostname available", 1
+ if $@ =~ /Cannot get host name/;
+ isnt($host, undef, "got a hostname");
+}
+
+{
+ use warnings;
+ my $warn;
+ local $SIG{__WARN__} = sub { $warn = "@_" };
+ eval { hostname("dummy") };
+ ok($warn, "warns with an argument");
+ like($warn, qr/hostname\(\) doesn't accept any arguments/,
+ "appropriate message");
+ no warnings "deprecated";
+ undef $warn;
+ eval { hostname("dummy") };
+ is($warn, undef, "no warning when disabled");
}
--
2.1.4
|
From @epaThanks for writing the patch. Are we sure that 'deprecated' is an appropriate warnings category? This is not a case of some existing feature which is scheduled to go away in the future. Arguments to hostname() have never done anything, now or in the past. Even if you don't care about making your code compatible with some future version of perl (which is the purpose of deprecation warnings) you would probably still want to find out about confused code which calls hostname with useless arguments. |
From @tonycozOn Tue Nov 17 02:19:04 2015, ed wrote:
I have no particular preference: it's what rjbs asked for. Tony |
From @ap* Ed Avis via RT <perlbug-followup@perl.org> [2015-11-17 11:20]:
Yes it is – assuming I read rjbs’ quite implicit comment correctly. He |
From @epaThanks for your views. The question of which exact warning category to use can get a bit rabbinical. The important thing is that there be a warning of some kind. So rjbs's patch looks good. |
From @ap* Ed Avis via RT <perlbug-followup@perl.org> [2015-11-18 15:30]:
Maybe so; but not in this case:
There is going to be not just a warning, but an exception. The warning
That was Tony Cook’s patch. :-) |
From @rjbs* Aristotle Pagaltzis <pagaltzis@gmx.de> [2015-11-17T14:24:37]
I believe that's what I meant at the time. I am now second-guessing. Here's Existing syntax and semantics should only be marked for destruction in You can read the behavior as "standing in the way of improvement" through a I think our options are at least these: 1 do nothing 2 deprecate with an explanation not covered by the policy above 3 deprecate, claiming that the policy above covers it 4 issue an unconditional, but not-deprecation-category warning with warn 5 issue a warnings::warn and register a Sys::Hostname category Let's have a quick check in from people who have an opinion on this? I'm -- |
From @tonycozOn Wed, Nov 18, 2015 at 10:30:01PM -0500, Ricardo Signes wrote:
Or: 6 make it croak, per the original discussion, justified as a bug fix. I don't have a strong opinion in any direction, I just bookmarked it Tony |
From zefram@fysh.orgRicardo Signes wrote:
I think this is appropriate. The deprecation policy that you If, contrary to this opinion, we don't actually deprecate the erroneous -zefram |
From @demerphqOn 19 November 2015 at 04:47, Zefram <zefram@fysh.org> wrote:
I agree. Were Sys::Hostname a cpan module I think there would be Yves -- |
From @epaThe text about deprecation talks about 'existing syntax and semantics' but this is vague. By some definition any language behaviour including the most horrible bug is 'existing semantics' and any construct written in a current Perl program is 'existing syntax'. I think it would be better for the deprecation policy to make it clear that it applies to existing *documented* behaviour or to that which, although not explicitly documented, is widely and deliberately used. On the other hand if 'deprecation' includes any kind of warning which may later become a fatal error, that could be made clear too. Another way to look at it is that deprecation covers things which are a working part of the language today, but will be withdrawn in a future version. If you don't care about future versions you could turn off deprecation warnings. But even in that case you would probably want to find out about odd calls like hostname('x') since they are mistakes *now*, and always have been, regardless of what may happen in a future release. By contrast a true deprecation is something which is correct code today. For these reasons I felt that a non-deprecation warning was best. There is no existing language feature which is being removed; arguments to hostname have never done anything useful. But I am content with any warning category. |
From @AbigailOn Thu, Nov 19, 2015 at 01:56:47AM -0800, Ed Avis via RT wrote:
If arguments to hostname() have never done anything useful, how could Are there any future plans for hostname() to actually return something Abigail |
From zefram@fysh.orgAbigail wrote:
hostname() is documented to take no parameters. Passing any parameters is -zefram |
From @ap* Zefram <zefram@fysh.org> [2015-11-19 12:35]:
It is not. The documentation is mute on the subject of arguments.
It is also never incorrect. The documentation does not propose any (Though even if it did, the bug might be what the documentation says
That is a correct red herring. |
From zefram@fysh.orgAristotle Pagaltzis wrote:
The documentation shows it called with no parameters, and does not show -zefram |
From @rjbs* Ricardo Signes <perl.p5p@rjbs.manxome.org> [2015-11-18T22:30:01]
I've given this thought and I think we should go with #2. If Sys-Hostname was an upstream-CPAN distribution, I would allow this change as Upstream-blead dists should not be treated differently from upstream-CPAN -- |
From @epaCould this bug be revisited? There was a consensus that one way or another, hostname() should start to warn when passed arguments. |
From @khwilliamsonOn Sun, 15 Oct 2017 21:45:11 -0700, tonyc wrote:
Should this go into perldeprecation? Somehow ISTR that deprecation messages are now supposed to include a definite deadline release in their text, but I don't see that written down |
From zefram@fysh.orgKarl Williamson via RT wrote:
Yes.
With the deprecation warning in 5.28 and 5.30, the minimum possible -zefram |
From @xsawyerxOn 12/05/2017 08:37 PM, Zefram wrote:
Yes.
Agreed. |
From zefram@fysh.orgDeprecation dated and documented in commit -zefram |
@xsawyerx - Status changed from 'open' to 'pending release' |
From @khwilliamsonThank you for filing this report. You have helped make Perl better. With the release yesterday of Perl 5.28.0, this and 185 other issues have been Perl 5.28.0 may be downloaded via: If you find that the problem persists, feel free to reopen this ticket. |
@khwilliamson - Status changed from 'pending release' to 'resolved' |
Migrated from rt.perl.org#124349 (status was 'resolved')
Searchable as RT124349$
The text was updated successfully, but these errors were encountered: