Skip to content

Scala3doc: Add missing givens, create pages for givens, minor bugfixes #10306

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

Merged
merged 1 commit into from
Nov 16, 2020

Conversation

pikinier20
Copy link
Contributor

@pikinier20 pikinier20 commented Nov 13, 2020

It's a PR fixing bug reported in #236

@pikinier20
Copy link
Contributor Author

cc @romanowski @abgruszecki

@pikinier20 pikinier20 changed the title Add missing givens, create pages for givens, minor bugfixes Scala3doc: Add missing givens, create pages for givens, minor bugfixes Nov 13, 2020
@@ -111,25 +111,55 @@ trait ClassLikeSupport:
val target = ExtensionTarget(extSym.symbol.name, extSym.tpt.dokkaType.asSignature, extSym.tpt.symbol.dri)
parseMethod(dd.symbol, kind = Kind.Extension(target))
}

// TODO check given methods?
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe we can check given methods as part of this PR?

case dd: DefDef if !dd.symbol.isHiddenByVisibility && dd.symbol.isGiven =>
Some(parseMethod(dd.symbol, kind = Kind.Given(getGivenInstance(dd).map(_.asSignature), None))) // TODO check given methods?
Some(dd.symbol.owner.typeMember(dd.name))
Copy link
Contributor

Choose a reason for hiding this comment

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

Can dd.symbol.owner.typeMember(dd.name) be a null? If not then simple if will be nicer to read:

if dd.symbol.owner.typeMember(dd.name).exisits then None
else Some(parseMethod(dd.symbol, kind = Kind.Given(getGivenInstance(dd).map(_.asSignature), None)))


case dd: DefDef if !dd.symbol.isHiddenByVisibility && !dd.symbol.isGiven && !dd.symbol.isSyntheticFunc && !dd.symbol.isExtensionMethod =>
Some(parseMethod(dd.symbol))

case td: TypeDef if !td.symbol.flags.is(Flags.Synthetic) && (!td.symbol.flags.is(Flags.Case) || !td.symbol.flags.is(Flags.Enum)) =>
Some(parseTypeDef(td))

case vd: ValDef if !isSyntheticField(vd.symbol)
&& (!vd.symbol.flags.is(Flags.Case) || !vd.symbol.flags.is(Flags.Enum))
Copy link
Contributor

Choose a reason for hiding this comment

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

maybe we can exctract all this conditions to dedicated method?

case t: TypeTree => Some(t.tpe)
case _ => None
}
val modifiedClasslikeExtension = ClasslikeExtension.getFrom(parsedClasslike).map(_.copy(
Copy link
Contributor

Choose a reason for hiding this comment

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

if you first call get and then copy you can get rid of map

@@ -238,6 +268,7 @@ trait ClassLikeSupport:
val name = methodKind match
case Kind.Constructor => "this"
case Kind.Given(_, _) => methodSymbol.name.stripPrefix("given_")
case Kind.Extension(_) => methodSymbol.name.stripPrefix("extension_")
Copy link
Contributor

Choose a reason for hiding this comment

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

This should no longer be necessary, the encoding of extension methods has changed and their names AFAIU are no longer prefixed with extension_.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In the version of master that this PR was based on, there were still extension_ prefixes before every extension.

Kind.Implicit(Kind.Val, extractImplicitConversion(valDef.tpt.tpe))
else defaultKind
else defaultKind
Copy link
Contributor

Choose a reason for hiding this comment

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

Why the indent?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Without indent there, I've got:

[error] 373 |      else defaultKind
[error]     |      ^
[error]     |The start of this line does not match any of the previous indentation widths.

@@ -17,13 +17,13 @@ class ImplicitMembersExtensionTransformer(ctx: DokkaContext) extends Documentabl

def expandMember(outerMembers: Seq[Member])(c: Member): Member =
val companion = c match
case classlike: DClass => ClasslikeExtension.getFrom(classlike).flatMap(_.companion).map(classlikeMap)
case classlike: DClass => ClasslikeExtension.getFrom(classlike).flatMap(_.companion).flatMap(classlikeMap.get)
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it expected that classlikeMap would miss some classlikes?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ahh, actually I changed it on the beginning of implementing this PR because it was throwing exceptions, but I've checked and this change is unnecessary

case Kind.Given(Some(instance), _) => withGenerics
.text(" as ")
.signature(instance)
case _ => withGenerics
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't we have tests for this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's actually tested by givenSignatures as we had these signatures documented, but in different way (we were generating signature from DFunction instead of DClass).

Copy link
Contributor

@romanowski romanowski left a comment

Choose a reason for hiding this comment

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

Some style-related comment. @abgruszecki asked some good questions

@abgruszecki
Copy link
Contributor

It seems that in the meantime grouping of extension methods broke, regrettably: https://scala3doc.virtuslab.com/pr-10306/scala3/api/scala/opaques/array-ops/index.html

Once this is merged we should open a separate issue for that problem.

@romanowski
Copy link
Contributor

We've decided to create a follow up PR with all the fixes.

@romanowski romanowski merged commit 88541a3 into scala:master Nov 16, 2020
@abgruszecki
Copy link
Contributor

I created an issue for the extension methods here: #10329.

pikinier20 added a commit to pikinier20/dotty that referenced this pull request Nov 16, 2020
pikinier20 added a commit to pikinier20/dotty that referenced this pull request Nov 16, 2020
pikinier20 added a commit to pikinier20/dotty that referenced this pull request Nov 19, 2020
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.

3 participants