Display roles in HTML docs#4121
Display roles in HTML docs#4121JordanMartinez merged 57 commits intopurescript:masterfrom JordanMartinez:addRolesToDocs
Conversation
|
Here's the control flow for the rendering pipeline
If we updated However, there are multiple uses of |
|
Finally getting back to this. I believe the final HTML and CSS we'll use will look like this (except all one one line rather than multiple lines like the example below). Here's a breakdown of what's happening:
<a href="#">
<abbr title="The 'nominal' role means this type cannot be safely coerced into another type">
<sub class="role__nominal"></sub>
</abbr>
</a>.role__nominal::after {
content: "nominal";
}
.role__phantom::after {
content: "phantom";
}Below is a screenshot of HalogenM with a few roles added ad-hoc where I'm hovering over the first one. Do we want to style the role annotations differently than what I'm showing below? |
|
Looks good to me! I would only suggest for wording: “… this type argument cannot be safely coerced into another type”, so it’s clear we are talking about the arguments, not actual types. Maybe there’s even a clearer way to phrase it to show the relationship of type arguments/roles to coercing the entire type. |
|
Re wording: how about: ‘... means this argument may not change when coercing this type’ and ‘... means this argument can change freely when coercing this type’ for nominal and phantom respectively? Re style: this is all highly subjective, but I propose:
I think maintaining the convention that fixed-width means code is important, and I think the italics help visually clarify the distinction when fixed-width and proportional fonts are mixed. |
|
For wording, I went with this before I saw @rhendric's comment above: describeNominal = fold
[ "The 'nominal' role means the type represented by this type "
, "parameter CANNOT be safely coerced to another type when one "
, "coerces the overall type to another type. For example, given "
, "`data Foo a` where `a` has the nominal role, the type, "
, "`Foo Age`, cannot be safely coerced to the type, "
, "`Foo Int`."
]
describePhantom = fold
[ "The 'phantom' role means the type represented by this type "
, "parameter CAN be safely coerced to any other type when one "
, "coerces the overall type to another type. For example, given "
, "`data Foo a` where `a` has the phantom role, the type, "
, "`Foo Age`, can be safely coerced to the type, "
, "`Foo Int`, or even a non-sensical type like, "
, "`Foo (Foo (Foo String))`."
]
While it is subjective, I agree with your idea. Part of the reason why I asked was I, too, thought the underline unneeded. |
|
I don't think we need ‘for example’s in the tooltips. They're already links; if a quick reminder isn't sufficient, the user can click. |
|
Given module Main where
data Foo :: Type -> Type -> Type -> Type
data Foo a b c = Foo
type role Foo nominal phantom representationalthe output of Also, I'm currently linking to Roles.md. Is that the URL we want to use? |
|
Oops, sorry, also I think the roles should be normal weight and not bold. (It was much less obvious that they were bold in the other font!) |
|
So, this PR is about ready for review. There's a few remaining questions:
|
|
CI started hanging due to an internal error. Getting #4126 in will stop that. |
|
CI passes now. |
|
Given the below code... module Main where
data DataAll :: Type -> Type -> Type -> Type
data DataAll a b c = DataAll
type role DataAll nominal phantom representational
data TypeOnly_UninterestingKind_InferRole :: Type -> Type -> Type
data TypeOnly_UninterestingKind_InferRole a b
data TypeOnly_InterestingKind_InferRole :: Type -> (Type -> Type) -> Type
data TypeOnly_InterestingKind_InferRole a b
foreign import data FFI_UninterestingKind_InferRole :: Type -> Type -> Type
foreign import data FFI_InterestingKind_InferRole :: forall k. k -> Symbol -> Type
foreign import data FFI_UninterestingKind_ExplicitRole :: Type -> Type -> Type
type role FFI_UninterestingKind_ExplicitRole phantom nominal
foreign import data FFI_InterestingKind_ExplicitRole :: forall k. k -> Symbol -> Type
type role FFI_InterestingKind_ExplicitRole phantom phantom |
|
I realized I was only inserting the inferred roles for FFI declarations. Latest commit also inserts inferred roles into data declarations. |
|
Synonyms are not supported in foreign data types kinds: we render for foreign import data FFI1 :: (Function Type) Typebut for type To = Function
foreign import data FFI1 :: (To Type) TypeThis is perhaps too uncommon to be dealt with in this pull request but we could do so by building a SynonymMap, like we do when desugaring instances, and then call Language.PureScript.TypeChecker.Synonyms.replaceAllTypeSynonymsM in convertFFIDecl. |
Yeah... that sounds like it should be in its own PR. |
tests/TestDocs.hs
Outdated
|
|
||
| , ShouldHaveRoleAnnotation (n "RoleAnnotationDocs") "FFI_HeadParens" [P.Representational, P.Nominal, P.Phantom] | ||
| -- , ShouldHaveRoleAnnotation (n "RoleAnnotationDocs") "FFI_TailParens" [P.Representational, P.Nominal, P.Phantom] | ||
| -- , ShouldHaveRoleAnnotation (n "RoleAnnotationDocs") "FFI_WholeParens" [P.Representational, P.Nominal, P.Phantom] |
|
@kl0tl I generated markdown docs to see whether the role annotations are rendered correctly. Looks like we can't use the HTML tags where I put them because the entire declaration is wrapped in a code block. Given module Main where
data Foo a = Foo a
type role Foo nominalwe get (horizontal lines added to clearly indicate start/finish of docs): Module Main
|
kl0tl
left a comment
There was a problem hiding this comment.
Looks like we can't use the HTML tags where I put them because the entire declaration is wrapped in a code block.
Oh, let’s not bother with roles in Markdown docs then.
…, is there any other work to do here besides waiting for #4157 to get merged and then adding additional tests?
It looks quite good to me modulo the Markdown docs, sorry about that!
…nstants" This reverts commit 0623409.
|
The markdown docs commit has been reverted and I fixed the typo you found. |
|
CI passes. |
|
If no one has any further feedback on this, I'm going to merge this in two days. |








Description of the change
Fixes #4116
WIP but should help continue the discussion already started.Checklist:
Current changes made in this PR
AST.Exported.exportedDeclarations[Role]argumentI haven't yet tried to render the roles in the HTML docs for a few reasons. First, we're still coming to an agreement about how we want to do this (though the subscript idea seems to be favored). Second, I'm still figuring out how the rendering code works and how I would insert role subscripts.
I'll ask other questions in the code itself.