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

SI-9620: add doc annotation to hide specific conversions #4952

Merged
merged 1 commit into from Feb 9, 2016

Conversation

Projects
None yet
6 participants
@felixmulder
Contributor

felixmulder commented Feb 8, 2016

This commit will introduce the doc annotation @hideImplicitConversion.
By specifying which conversions to hide, the user can "toggle" which
conversions are shown be default.

This implementation is a better workaround than hardcoding which ones to
ignore when running scaladoc.

An example of how to use this:

package a

trait Foo[S] {
  def foo(t: S): Int = 123
}

/** Boo with only one foo method, hopefully!
 * @hideImplicitConversion BooShouldNotAppearIsFoo
 */
trait Boo[T]

object Boo {
  sealed trait ShouldNotAppear
  implicit class BooShouldNotAppearIsFoo(boo: Boo[ShouldNotAppear]) extends Foo[ShouldNotAppear]
  implicit class BooLongIsFoo(boo: Boo[Long]) extends Foo[Long]
}

Review: @VladUreche

@scala-jenkins scala-jenkins added this to the 2.12.0-M4 milestone Feb 8, 2016

@felixmulder

This comment has been minimized.

Show comment
Hide comment
@felixmulder

felixmulder Feb 9, 2016

Contributor

All right - updated with your input @VladUreche

Contributor

felixmulder commented Feb 9, 2016

All right - updated with your input @VladUreche

@VladUreche

This comment has been minimized.

Show comment
Hide comment
@VladUreche

VladUreche Feb 9, 2016

Member

Thanks for the quick update Felix and sorry for my belated answers.

Thinking more about this, I'm not sure it will eliminate the Array problem. The reason is that it hides the implicit conversions at the very last moment, when the html is generated. But the JSON is still generated with the multiple members.

At the moment, I guess your previous PR which amended the hardcoded object masks this problem. But I expect once you remove the hardcoding, you will see the duplicate methods in search again.

So, the way I think this should work is to prune the implicit conversions directly in ModelFactoryImplicitSupport, before any implicitly added members are created for the current entity. WDYT?

Member

VladUreche commented Feb 9, 2016

Thanks for the quick update Felix and sorry for my belated answers.

Thinking more about this, I'm not sure it will eliminate the Array problem. The reason is that it hides the implicit conversions at the very last moment, when the html is generated. But the JSON is still generated with the multiple members.

At the moment, I guess your previous PR which amended the hardcoded object masks this problem. But I expect once you remove the hardcoding, you will see the duplicate methods in search again.

So, the way I think this should work is to prune the implicit conversions directly in ModelFactoryImplicitSupport, before any implicitly added members are created for the current entity. WDYT?

@felixmulder

This comment has been minimized.

Show comment
Hide comment
@felixmulder

felixmulder Feb 9, 2016

Contributor

Damnit, you're right!

Regarding pruning in ModelFactoryImplicitSupport, this will remove the entries completely. Are we fine with that? I mean - the current solution in this PR hides them from view - pruning them in ModelFactoryImplicitSupport will completely remove them.

So IMO there are two options:

  • Prune in ModelFactoryImplicitSupport (they won't show up)
  • Keep the current implementation, but also prune in IndexScript.scala where the JSON is generated (they won't show up in search, but will be toggle-able on the entity page)

WDYT?

Contributor

felixmulder commented Feb 9, 2016

Damnit, you're right!

Regarding pruning in ModelFactoryImplicitSupport, this will remove the entries completely. Are we fine with that? I mean - the current solution in this PR hides them from view - pruning them in ModelFactoryImplicitSupport will completely remove them.

So IMO there are two options:

  • Prune in ModelFactoryImplicitSupport (they won't show up)
  • Keep the current implementation, but also prune in IndexScript.scala where the JSON is generated (they won't show up in search, but will be toggle-able on the entity page)

WDYT?

@VladUreche

This comment has been minimized.

Show comment
Hide comment
@VladUreche

VladUreche Feb 9, 2016

Member

Yes, both solutions do the trick. Though I'm more in favor of the ModelFactoryImplicitSupport one, which looks more principled.

Member

VladUreche commented Feb 9, 2016

Yes, both solutions do the trick. Though I'm more in favor of the ModelFactoryImplicitSupport one, which looks more principled.

@felixmulder

This comment has been minimized.

Show comment
Hide comment
@felixmulder

felixmulder Feb 9, 2016

Contributor

All right - then I'll go for # 1 👍

Contributor

felixmulder commented Feb 9, 2016

All right - then I'll go for # 1 👍

@felixmulder

This comment has been minimized.

Show comment
Hide comment
@felixmulder

felixmulder Feb 9, 2016

Contributor

At first I thought I couldn't get the comment from the ModelFactoryImplicitSupport class - then I noticed the thisFactory variable and the party was back on 🎉

This should be along the lines of what you intended for the implementation @VladUreche

Contributor

felixmulder commented Feb 9, 2016

At first I thought I couldn't get the comment from the ModelFactoryImplicitSupport class - then I noticed the thisFactory variable and the party was back on 🎉

This should be along the lines of what you intended for the implementation @VladUreche

@felixmulder

This comment has been minimized.

Show comment
Hide comment
@felixmulder

felixmulder Feb 9, 2016

Contributor

Pulled changes from upstream into my branch - once it's been through Jenkins it should be good to go.

Contributor

felixmulder commented Feb 9, 2016

Pulled changes from upstream into my branch - once it's been through Jenkins it should be good to go.

@VladUreche

This comment has been minimized.

Show comment
Hide comment
@VladUreche

VladUreche Feb 9, 2016

Member

@felixmulder great! Can you please add a test case on this?
You can use this as a template: https://github.com/scala/scala/blob/2.12.x/test/scaladoc/run/SI-3448.scala

Member

VladUreche commented Feb 9, 2016

@felixmulder great! Can you please add a test case on this?
You can use this as a template: https://github.com/scala/scala/blob/2.12.x/test/scaladoc/run/SI-3448.scala

@VladUreche

This comment has been minimized.

Show comment
Hide comment
@VladUreche

VladUreche Feb 9, 2016

Member

Maybe a good think to test is that adding this annotation to Array and un-hardcoding the implicit conversions to be ignored produces the right scaladoc output.

Member

VladUreche commented Feb 9, 2016

Maybe a good think to test is that adding this annotation to Array and un-hardcoding the implicit conversions to be ignored produces the right scaladoc output.

@felixmulder

This comment has been minimized.

Show comment
Hide comment
@felixmulder

felixmulder Feb 9, 2016

Contributor

I've added the test and I'll look at Array now. Will ping you once I know that everything's working 😄

Contributor

felixmulder commented Feb 9, 2016

I've added the test and I'll look at Array now. Will ping you once I know that everything's working 😄

@VladUreche

This comment has been minimized.

Show comment
Hide comment
@VladUreche

VladUreche Feb 9, 2016

Member

👍

Member

VladUreche commented Feb 9, 2016

👍

@felixmulder

This comment has been minimized.

Show comment
Hide comment
@felixmulder

felixmulder Feb 9, 2016

Contributor

Ok - so this is where I'm at. I've added the annotation to the Array class. Removed the hardcoded ignores from the Settings.scala file in scaladoc. I've created a test for SI-9620.

It generates the Array documentation without the implicits, huzzah! 🎉

HOWEVER! I can't get my stupid test to work 😢

@VladUreche

Contributor

felixmulder commented Feb 9, 2016

Ok - so this is where I'm at. I've added the annotation to the Array class. Removed the hardcoded ignores from the Settings.scala file in scaladoc. I've created a test for SI-9620.

It generates the Array documentation without the implicits, huzzah! 🎉

HOWEVER! I can't get my stupid test to work 😢

@VladUreche

// Assert Boo only has one implicit conversion
val boo = rootPackage._package("a")._trait("Boo")
val conversions = boo._conversions("a.Boo.BooShouldNotAppearIsFoo") ++ boo._conversions("a.Boo.BooLongIsFoo")
assert(conversions.length == 1, conversions.length + " == 1")

This comment has been minimized.

@felixmulder

felixmulder Feb 9, 2016

Contributor

@VladUreche: the length here is 0 for some reason. I also checked boo.conversions.length which is 0.

@felixmulder

felixmulder Feb 9, 2016

Contributor

@VladUreche: the length here is 0 for some reason. I also checked boo.conversions.length which is 0.

This comment has been minimized.

@VladUreche

VladUreche Feb 9, 2016

Member

I think it's probably the fact that -implicits is not set in the scaladoc flags.

@VladUreche

VladUreche Feb 9, 2016

Member

I think it's probably the fact that -implicits is not set in the scaladoc flags.

This comment has been minimized.

@VladUreche
@VladUreche

VladUreche Feb 9, 2016

Member
Show outdated Hide outdated test/scaladoc/run/SI-9620.scala Outdated
SI-9620: add doc annotation to hide specific conversions
This commit will introduce the doc annotation `@hideImplicitConversion`.
By specifying which conversions to hide, the user can "toggle" which
conversions are kept in the parsed entity.

This implementation is a better workaround than hardcoding which ones to
ignore when running scaladoc.

Review: @VladUreche
@felixmulder

This comment has been minimized.

Show comment
Hide comment
@felixmulder

felixmulder Feb 9, 2016

Contributor

@VladUreche thanks so much! It works 😄 Thanks a bunch for the help!

Squished and commited!

Contributor

felixmulder commented Feb 9, 2016

@VladUreche thanks so much! It works 😄 Thanks a bunch for the help!

Squished and commited!

@VladUreche

This comment has been minimized.

Show comment
Hide comment
@VladUreche

VladUreche Feb 9, 2016

Member

LGTM, thanks a lot for your tireless work @felixmulder!
Ping @janekdb: can you please add the hideImplicitConversion to the scaladoc tutorial?

Member

VladUreche commented Feb 9, 2016

LGTM, thanks a lot for your tireless work @felixmulder!
Ping @janekdb: can you please add the hideImplicitConversion to the scaladoc tutorial?

@janekdb

This comment has been minimized.

Show comment
Hide comment
@janekdb

janekdb Feb 9, 2016

Member

@VladUreche : Added to pending list on scala/scala-lang#394

Member

janekdb commented Feb 9, 2016

@VladUreche : Added to pending list on scala/scala-lang#394

VladUreche added a commit that referenced this pull request Feb 9, 2016

Merge pull request #4952 from felixmulder/topic/scaladoc-hideImplicit…
…Conversions-tag

SI-9620: add doc annotation to hide specific conversions

@VladUreche VladUreche merged commit b0beb72 into scala:2.12.x Feb 9, 2016

5 checks passed

cla @felixmulder signed the Scala CLA. Thanks!
Details
integrate-ide [1008] SUCCESS. Took 2 s.
Details
validate-main [1222] SUCCESS. Took 60 min.
Details
validate-publish-core [1199] SUCCESS. Took 3 min.
Details
validate-test [1001] SUCCESS. Took 57 min.
Details
@VladUreche

This comment has been minimized.

Show comment
Hide comment
@VladUreche

VladUreche Feb 9, 2016

Member

👍

Member

VladUreche commented Feb 9, 2016

👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment