Make .WHY on role group delegate to default role #3549
Conversation
|
My sense of DWIM likes the change. With regard to spectest: this can be |
|
The 3 failing spec tests all have the same description "Role group's WHY should not be defined" and were committed by the @hoelzro around the same time. They seem to be testing the same thing in different settings (leading and trailing docs). I'm pretty sure they result from this single example in the speculations: This is what the tests were meant to establish. Note that this one sentence does not actually prohibit the role group from delegating to the default role. It merely requires that individual roles can be documented. This commit does not violate that requirement. It just extends availability of the default role's docs to the role group which as the linked issue makes pretty clear is what people expect (because they usually don't think in terms of role groups). I think the failing spec tests are simply overly specific and adapting them so this can be merged will do much more good than harm. I only see potential for a little bit of confusion of tools, but since @jnthn is directly involved with the current major consumer (Comma-IDE), he'll know best about this anyway. Even if there is any confusion at all, tools would have to be adapted anyway to deal with the next language version and then maybe even differentiate between versions. So it's not just us who'd have to carry code to hide this feature in old versions, but those other tools as well. I'm pretty sure that's not worth the hassle and it's better to merge this as-is. |
|
Fixed the conflict, is this ready to be merged? |
|
@vrurg Sure, go for it. 🙏 |
Roles have a two-level structure: the role name is a parametric role group, which in turns contains the parametric role. In the case of there being many roles of the same short name, they are within the one group. Declarator docs correctly attach to the parametric role. However, that means .WHY on the group doesn't actually result in those docs. You have to extract the candidate from the group. In many cases, however, we people ain't using parametric roles, and we make many meta-operations delegate to the single or unsignatured role candidate, if any. We do not currently do this for .WHY. This commit makes us do so, BUT it turns out there are 3 current spectests that will fail because they explicitly expect that not to happen. Thus, we need to make a decision over whether we want this change. Relates to #3521.
- Remove `Perl6::Metamodel::Documenting` from consumed roles - Fully implement `WHY`, `set_why` - Make `set_why` throw `X::Role::Group::Documenting` as a safety measure I deliberately don't delegate `set_why` in order to avoid accidental overriding of parametric role documentation. It doesn't and must not happen in Rakudo implementation anyway.
Because signature is a local scope from documentation point of view.
Fixes documenting of parametric role:
role R[::T] {
#= this now belongs to the role, not to the parameter T
}
And makes one spec TODO passing.
vrurg
force-pushed
the
WHY-on-role-group
branch
from
01567ff
to
8f989d1
Compare
This required to remove tests which were testing for `Role.WHY` returning `Nil`. But as was suggested in this comment: rakudo/rakudo#3549 (comment) the tests were needed to ensure that a documentation comment is attached to a particular parametric role, not to its group. But since rakudo/rakudo#3549 makes `.WHY` delegate to the default candidate, these tests doesn't make sense anymore. Instead why-both.t now makes sure what we get from a role group is the same object we get from the default candidate. Alongside with the essential changes, also unified all tests with regard to use of idential test assertion subs. They all now are `subtest` based.
It is passing since rakudo/rakudo#3549
|
I couldn't stop on just fixing the methods... Since a role group is not documentable, I removed the corresponding role from it. Spectests were adjusted to the new behavior in Raku/roast#783 by removing the requirement of Otherwise PR is ready for merge. The failure of Lin_MVM_spec CI can be ignored, it is the only one ran against the specs, which are not ready yet. |
This required to remove tests which were testing for `Role.WHY` returning `Nil`. But as was suggested in this comment: rakudo/rakudo#3549 (comment) the tests were needed to ensure that a documentation comment is attached to a particular parametric role, not to its group. But since rakudo/rakudo#3549 makes `.WHY` delegate to the default candidate, these tests doesn't make sense anymore. Instead why-both.t now makes sure what we get from a role group is the same object we get from the default candidate. Alongside with the essential changes, also unified all tests with regard to use of idential test assertion subs. They all now are `subtest` based.
This required to remove tests which were testing for `Role.WHY` returning `Nil`. But as was suggested in this comment: rakudo/rakudo#3549 (comment) the tests were needed to ensure that a documentation comment is attached to a particular parametric role, not to its group. But since rakudo/rakudo#3549 makes `.WHY` delegate to the default candidate, these tests doesn't make sense anymore. Instead why-both.t now makes sure what we get from a role group is the same object we get from the default candidate. Alongside with the essential changes, also unified all tests with regard to use of idential test assertion subs. They all now are `subtest` based.
Roles have a two-level structure: the role name is a parametric role
group, which in turns contains the parametric role. In the case of there
being many roles of the same short name, they are within the one group.
Declarator docs correctly attach to the parametric role. However, that
means .WHY on the group doesn't actually result in those docs. You have
to extract the candidate from the group.
In many cases, however, we people ain't using parametric roles, and we
make many meta-operations delegate to the single or unsignatured role
candidate, if any. We do not currently do this for .WHY. This commit
makes us do so, BUT it turns out there are 3 current spectests that will
fail because they explicitly expect that not to happen. Thus, we need to
make a decision over whether we want this change.
Relates to #3521.