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

Supress line number if throw X::Package::Stubbed #1154

Merged
merged 2 commits into from Sep 14, 2017
Merged

Supress line number if throw X::Package::Stubbed #1154

merged 2 commits into from Sep 14, 2017

Conversation

cuonglm
Copy link
Contributor

@cuonglm cuonglm commented Sep 11, 2017

No description provided.

method message() {
"The following packages were stubbed but not defined:\n "
~ @.packages.join("\n ");
}
multi method gist(::?CLASS:D: :$sorry = True, :$expect = True) {
Copy link
Member

Choose a reason for hiding this comment

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

Why are these named parameters being given default values but then never used?

Copy link
Contributor Author

@cuonglm cuonglm Sep 13, 2017

Choose a reason for hiding this comment

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

@jnthn Because I override one defined in role X::Comp

@jnthn
Copy link
Member

jnthn commented Sep 13, 2017

Does this still output the ==SORRY== before the error?

@cuonglm
Copy link
Contributor Author

cuonglm commented Sep 13, 2017

@jnthn No, as I added a test in Raku/roast#310

@cuonglm
Copy link
Contributor Author

cuonglm commented Sep 14, 2017

cc @jnthn @zoffixznet What can we do to make it better and clearer?

The only way I can think is about overriding the multi method gist, but it will make others confuse like @jnthn comment above.

@zoffixznet
Copy link
Contributor

cc @jnthn @zoffixznet What can we do to make it better and clearer?

I'm not sure what this PR is fixing, sorry. Don't we normally do want the line numbers?

The multi method gist(::?CLASS:D: :$sorry = True, :$expect = True) looks to me like it could just be multi method gist(). You mentioned you're overriding something, but I'm not seeing it. Looks like the arguments aren't used at all and need not be specified, unless you're doing some multi-dispatch tie-breaking.

@jnthn
Copy link
Member

jnthn commented Sep 14, 2017

Worth a mention: they're not needed 'cus all methods have an implicit *%_

@cuonglm
Copy link
Contributor Author

cuonglm commented Sep 14, 2017

@zoffixznet the TODO line said that line number should be suppressed. Yes, the other multi method was defined in X::Comp role, which I refered in response to @jnthn comment

@zoffixznet
Copy link
Contributor

zoffixznet commented Sep 14, 2017

@moritz added that comment 6 years ago... but I don't understand why we'd want to remove the line number.

From what I can tell so far, all this PR should be doing is simply removing that comment.

Worth a mention: they're not needed 'cus all methods have an implicit *%_

Seems in this case they do, thought I don't know if it's a bug. It can be shortened to just multi method gist(::?CLASS:D: :$) though

@moritz
Copy link
Member

moritz commented Sep 14, 2017

Back when I wrote that, the line number was likely one inside the compiler, or maybe the line number where the compunit closes; that is more confusing than helpful for the user.

@zoffixznet
Copy link
Contributor

Back when I wrote that, the line number was likely one inside the compiler,

Thanks. Now it shows the line number for where the stub is, so I'd say we don't want to suppress anything anymore. @Gnouc, please just remove the comment.

$ perl6 -I. -MFoo -e ''
===SORRY!=== Error while compiling /tmp/tmp.nnEFj6tntx/Foo.pm6 (Foo)
The following packages were stubbed but not defined:
    Foo
at /tmp/tmp.nnEFj6tntx/Foo.pm6 (Foo):4
------> <BOL>⏏<EOL>
    expecting any of:
        statement end

@moritz
Copy link
Member

moritz commented Sep 14, 2017

@zoffixznet is that a recent change? With rakudo 2017.08, I get:

moritz@pete:~$ cat stubbed.p6 
class A { ... }
# more stuff say

say 42;
moritz@pete:~$ perl6 stubbed.p6
===SORRY!=== Error while compiling /home/moritz/stubbed.p6
The following packages were stubbed but not defined:
    A
at /home/moritz/stubbed.p6:5
------> <BOL>⏏<EOL>

The error points at line 5, the end of the file, not line 1 where A was stubbed.

@zoffixznet
Copy link
Contributor

zoffixznet commented Sep 14, 2017

The error points at line 5, the end of the file, not line 1 where A was stubbed.

Oh, sorry, I didn't realize it could be pointing there. Yes, it points to the end of file for me as well :)

@Gnouc 👍 Thanks!

@zoffixznet zoffixznet merged commit fe71940 into rakudo:nom Sep 14, 2017
zoffixznet added a commit that referenced this pull request Sep 14, 2017
The actual params aren't used and aren't needed, but we still need
a bit of params to make the method more specific than the one
from the role.

See also:
#1154
https://irclog.perlgeek.de/perl6-dev/2017-09-14#i_15164490
@zoffixznet
Copy link
Contributor

Seems in this case they do, thought I don't know if it's a bug. It can be shortened to just multi method gist(::?CLASS:D: :$) though

I did that change in 7ba9b7cd6f

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.

None yet

4 participants