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

Add protected and private visibility filters to scaladoc #8183

Merged
merged 5 commits into from Jul 23, 2019

Conversation

exoego
Copy link

@exoego exoego commented Jun 27, 2019

Closes scala/bug#10999

This PR adds Protected and Private visiblity filters.
Private filter is available only if scaladoc -private used, so user can omit private members to reduce scaladoc size.
Existing All is removed since it is no longer required.
visibility-filters-toggle

@scala-jenkins scala-jenkins added this to the 2.13.1 milestone Jun 27, 2019
@exoego
Copy link
Author

exoego commented Jun 27, 2019

Currently, visibility modifiers are shown inside a full comment, which is expanded if user clicked a member.
I think it would be more straightforward if visibility modifiers are shown always to the left of member., like final do so.
Anyway, I don't address this since it is a different topic.
image

@exoego
Copy link
Author

exoego commented Jul 1, 2019

Travis passed successfully, but checks on GitHub are not updated...

Could someone rerun Travis or perform something to resolve checks?

Copy link
Member

@lrytz lrytz left a comment

Choose a reason for hiding this comment

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

Since we're now adding scaladocs for all private members, the doc size and the time to run scaladoc might grow significanlty.

Could you compare the scaladocs (size, time to generate) for the library and compiler, so we have an idea about the impact?

@@ -1018,7 +1018,9 @@ class ModelFactory(val global: Global, val settings: doc.Settings) {
aSym.info.members.exists(s => localShouldDocument(s) && (!s.isConstructor || s.owner == aSym))

def localShouldDocument(aSym: Symbol): Boolean =
!aSym.isPrivate && (aSym.isProtected || aSym.privateWithin == NoSymbol) && !aSym.isSynthetic
((aSym.isPrivate && !aSym.isTopLevel) ||
(!aSym.isPrivate && (aSym.isProtected || aSym.privateWithin == NoSymbol))) &&
Copy link
Member

Choose a reason for hiding this comment

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

I don't understand what the privateWithin check is for, can we remove it? As far as I know, privateWithin can be set in private[x] and protected[x]. The line excludes any private, and includes any protected, so the privateWithin seems useless, no? Maybe just try if all tests pass if you remove it.

Copy link
Author

@exoego exoego Jul 1, 2019

Choose a reason for hiding this comment

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

Unfortunately, removing aSym.privateWithin == NoSymbol breaks the following tests.
(I have not investigated fully why broken)

[info] Elapsed time: 0.045 sec 
[info] ! HtmlFactory.scala/bug#8144: Members' permalink - inner package: Falsified after 0 passed tests.
[info] Elapsed time: 0.000 sec 
[info] ! HtmlFactory.scala/bug#8144: Members' permalink - companion object: Falsified after 0 passed tests.
[info] Elapsed time: 0.001 sec 
[info] ! HtmlFactory.scala/bug#8144: Members' permalink - class: Falsified after 0 passed tests.
[info] Elapsed time: 0.000 sec 
[info] ! HtmlFactory.scala/bug#9599 Multiple @todo formatted with comma on separate line: Falsified after 0 passed tests.
[info] Elapsed time: 0.063 sec 
[info] ! HtmlFactory.scala/bug#10999 Private and Protected method should also be documented: Falsified after 0 passed tests.

Copy link
Member

Choose a reason for hiding this comment

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

Sorry, I was not very clear.. I pushed a change that simplifies localShouldDocument, hopefully that passes the tests.

Copy link
Member

Choose a reason for hiding this comment

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

OK, here is my misunderstanding:

class C {
  private def a
  private[C] def b
  protected def c
  protected[C] def d
}

We get

isPrivate isProtected privateWithin
a true false NoSymbol
b false false C
c false true NoSymbol
d false true C

Note that for b, isPrivate is false, but for d, isProtected is true.

@dwijnand

This comment has been minimized.

@exoego
Copy link
Author

exoego commented Jul 1, 2019

Since we're now adding scaladocs for all private members, the doc size and the time to run scaladoc might grow significanlty.

Could you compare the scaladocs (size, time to generate) for the library and compiler, so we have an idea about the impact?

Summary (see below comparison for detail)

  • Size increases 10-13%.
  • Time-to-generate increases 7-8%.

Ofcourse these varies depending on how many private members in project, I think this is not so expensive for most projects.

Scala Library

mkdir docs
cd docs
time ../build/pack/bin/scaladoc   ../src/library/**/*.scala
du -K scala/
  no-private private diff
size (KB) 186468 205584 +10%
time to generate (s) 53.75 64.01  
  55.7 62.03  
  55.55 56.32  
  54.11 55.48  
  57.05 58  
avg. (s) 55.232 59.168 +7%

Scala Compiler

mkdir docs
cd docs
time ../build/pack/bin/scaladoc   ../src/compiler/**/*.scala
du -K scala/
  no-private private  
size (KB) 93240 105200 +13%
time to generate (s) 46.03 46.72  
  44.36 53.18  
  45.7 46.99  
  46.27 48.88  
  46.74 52.78  
avg. (s) 45.82 49.71 +8%

@lrytz
Copy link
Member

lrytz commented Jul 2, 2019

Thank you for gathering these numbers. I think we should keep this PR open for discussion. Personally I don't see a good use case for listing private members in Scaladocs, so I'm not in favor. How about introducing a scaladoc option, e.g., -privates?

@diesalbla diesalbla added the tool:scaladoc Changes to ScalaDoc, the API documentation generator label Jul 2, 2019
@SethTisue
Copy link
Member

I don't see a good use case for listing private members in Scaladocs

is there a precedent, one way or the other, in Javadoc?

@exoego
Copy link
Author

exoego commented Jul 3, 2019

Javadoc can specify members to be included via the switch

  • -public (public only)
  • -protected (public + protected)
  • -private (public + protected + private)

You can find switches via javadoc -help.

@exoego
Copy link
Author

exoego commented Jul 3, 2019

Some test (reflect/compile:doc) fails after 86f29be.

[error] - doc
[error]   - reflect/compile:doc failed: java.lang.IllegalArgumentException: 'type TreeCopier' isn't a class, trait or object thus cannot be built as a member template.
[error] java.lang.RuntimeException
[error] 	at $666274811a7807c56755$.$anonfun$root$26(build.sbt:955)
[error] 	at $666274811a7807c56755$.$anonfun$root$26$adapted(build.sbt:894)
[error] 	at scala.Function1.$anonfun$compose$1(Function1.scala:44)
[error] 	at sbt.internal.util.$tilde$greater.$anonfun$$u2219$1(TypeFunctions.scala:40)
[error] 	at sbt.std.Transform$$anon$4.work(System.scala:67)
[error] 	at sbt.Execute.$anonfun$submit$2(Execute.scala:269)
[error] 	at sbt.internal.util.ErrorHandling$.wideConvert(ErrorHandling.scala:16)
[error] 	at sbt.Execute.work(Execute.scala:278)
[error] 	at sbt.Execute.$anonfun$submit$1(Execute.scala:269)
[error] 	at sbt.ConcurrentRestrictions$$anon$4.$anonfun$submitValid$1(ConcurrentRestrictions.scala:178)
[error] 	at sbt.CompletionService$$anon$2.call(CompletionService.scala:37)
[error] 	at java.util.concurrent.FutureTask.run(FutureTask.java:266)
[error] 	at java.util.concurrent.Executors$RunnableAdapter.call(Executors.java:511)
[error] 	at java.util.concurrent.FutureTask.run(FutureTask.java:266)
[error] 	at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1149)
[error] 	at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:624)
[error] 	at java.lang.Thread.run(Thread.java:748)
[error] (testAll) java.lang.RuntimeException

@lrytz
Copy link
Member

lrytz commented Jul 4, 2019

I'll take a look

@lrytz
Copy link
Member

lrytz commented Jul 4, 2019

@exoego a -private seems to be a good solution. Could you do that?

@lrytz
Copy link
Member

lrytz commented Jul 22, 2019

@exoego let us know if you still have time to work on this one!

@exoego
Copy link
Author

exoego commented Jul 22, 2019

@lrytz Added -private settings

@exoego exoego force-pushed the fix-10999 branch 2 times, most recently from d36204c to ccdb4dd Compare July 22, 2019 21:48
@exoego exoego force-pushed the fix-10999 branch 2 times, most recently from c523e16 to cafd5b5 Compare July 23, 2019 07:07
Copy link
Member

@lrytz lrytz left a comment

Choose a reason for hiding this comment

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

Thank you, @exoego!

@lrytz lrytz merged commit 27134c7 into scala:2.13.x Jul 23, 2019
@SethTisue SethTisue added the release-notes worth highlighting in next release notes label Jul 23, 2019
@exoego exoego deleted the fix-10999 branch July 23, 2019 20:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release-notes worth highlighting in next release notes tool:scaladoc Changes to ScalaDoc, the API documentation generator
Projects
None yet
6 participants