Skip to content

Scaladoc diagrams (again)#816

Merged
jsuereth merged 7 commits intoscala:masterfrom
VladUreche:feature/diagrams-dev-pullreq-new
Jul 7, 2012
Merged

Scaladoc diagrams (again)#816
jsuereth merged 7 commits intoscala:masterfrom
VladUreche:feature/diagrams-dev-pullreq-new

Conversation

@VladUreche
Copy link
Contributor

Review by:
@kzys, @heathermiller

VladUreche and others added 7 commits July 2, 2012 13:34
Scaladoc can create raw content files that we can easily diff and spot
any modifications. There is a cool project by Stefan Zeiger to export
the scaladoc model in JSON, but with the language and scaladoc being so
quick to evolve, it'll be a pain to properly maintain. In the long-run,
the plan is to sample a couple of raw files on each build and email me
the diff. If I spot anything that may be wrong I can fix it, revert the
commit or at least file a bug.

For now, .html.raw files are generated on-demand, using
  ant -Dscaladoc.raw.output="yes" <targets>

Also added a script that will do the job of diff-ing.

Review by @jsuereth.

Conflicts:

	src/compiler/scala/tools/nsc/doc/Settings.scala
Since the old model was "interruptible", it was prone to something
similar to race conditions -- where the model was creating a template,
that template creating was interrupted to creat another template, and
so on until a cycle was hit -- then, the loop would be broken by
returning the originally not-yet-finished template.

Now everything happens in a depth-first order, starting from root,
traversing packages and classes all the way to members. The previously
interrupting operations are now grouped in two categories:
 - those that were meant to add entities, like inheriting a class from
a template to the other (e.g. trait T { class C }; trait U extends T)
=> those were moved right after the core model creation
 - those that were meant to do lookups - like finding the companion
object -- those were moved after the model creation and inheritance
and are not allowed to create new documentable templates.

Now, for the documentable templates we have:

DocTemplateImpl - the main documentable template, it represents a
                  Scala template (class, trait, object or package).
                  It may only be created when modelFinished=false by
                  methods in the modelCreation object
NoDocTemplateMemberImpl - a non-documented (source not present)
                          template that was inherited. May be used as
                          a member, but does not get its own page
NoDocTemplateImpl - a non-documented (source not present) template
                    that may not be used as a member and does not
                    get its own page

For model users: you can use anything in the ModelFactory trait at
will, but not from the modelCreation object -- that is reserved for the
core model creation and using those functions may lead to duplicate
templates, invalid links and other ugly problems.
This commit contains model changes required for adding class diagrams
to scaladoc. It also contains an improved implicit shadowing
computation, which hides the shadowed implicitly inherited members
from the main view and gives instructions on how to access them.

This is joint work with Damien Obrist (@damienobrist) on supporting
diagram generation in scaladoc, as part of Damien's semester project
in the LAMP laborarory at EPFL.

The full history is located at:
https://github.com/damienobrist/scala/tree/feature/diagrams-dev

Commit summary:

 - diagrams model
   - diagram settings (Settings.scala, ScalaDoc.scala)
   - diagram model object (Entity.scala, Diagram.scala)
   - model: tracking direct superclasses and subclasses,
     implicit conversions from and to (ModelFactory.scala)
   - diagram object computation (DiagramFactory.scala, DocFactory.scala)
   - capacity to filter diagrams (CommentFactory.scala,
     DiagramDirectiveParser.scala)
   - diagram statistics object (DiagramStats.scala)
   - delayed link evaluation (Body.scala, Comment.scala)
   - tests

 - improved implicits shadowing information
   - model shadowing computation (ModelFactoryImplicitSupport.scala,
     Entity.scala)
   - html generation for shadowing information (Template.scala)
   - tests

Also fixes an issue reported by @dragos, where single-line comment
expansion would lead to the comment disappearing.

Review by @kzys, @pedrofurla.

Adapted to the new model and fixed a couple of problems:
 - duplicate implicit conversions in StringAdd/StringFormat
 - incorrect implicit conversion signature (from X to X)

Conflicts:

	src/compiler/scala/tools/nsc/doc/Settings.scala
	src/compiler/scala/tools/nsc/doc/html/page/Template.scala
	src/compiler/scala/tools/nsc/doc/model/Entity.scala
	src/compiler/scala/tools/nsc/doc/model/ModelFactory.scala
	src/compiler/scala/tools/nsc/doc/model/ModelFactoryImplicitSupport.scala
This commit contains the svg diagram generation using the graphviz
package, the template changes, the css styling and javascript code
that enables displaying and interacting with the diagrams.

The full history is located at:
https://github.com/damienobrist/scala/tree/feature/diagrams-dev

The diagrams are included as svg markup inside the html code. This
enables interacting with the image beyond what would be possible
with a static image (highlighting, scaling, tooltips, links to
nodes, etc).

The svg generation has four main phases: model => dot,
dot => svg (using the graphviz package), svg postprocessing,
inclusion in the html page.

This commit also fixes SI-5212 - links to individual pages
automatically load the left navigation panel of the website.

Commit summary:

 - diagram generation
   - model => dot (DotDiagramGenerator.scala, DiagramGenerator.scala)
   - dot => svg (DotRunner.scala)
   - svg post-processing (DotDiagramGenerator.scala)
   - svg inclusion in the html (Template.scala)

 - diagram interaction
   - css, js and image files

Review by @heathermiller, @kzys.

Also fixed the memory leak that was causing the testsuite to timeout.
Since we used it in the DocRunner and noticed it could have better
documentation.

Review by @heathermiller.
- relaxed the restrictions on nodes - nodes can be classes, traits and
objects, both stand-alone and companion objects -- all are added to the
diagram, but usually companion objects are filtered out as they don't
have any superclasses
- changed the rules for default diagram creation:
  - classes and traits (and AnyRef) get inheritance diagrams
  - packages and objects get content diagrams
(can be overridden by @contentDiagram [hideDiagram] and
@inheritanceDiagram [hideDiagram])
- tweaked the model to register subclasses of Any
- hardcoded the scala package diagram to show all relations
- enabled @contentDiagram showInheritedNodes by default and changed
the setting to hideInheritedNodes (and added a test for this)
- better node selection (can select nodes that don't have a
corresponding trait)
- fixed the docsite link in member selection, which was broken since
the first commit :))
- fixed the AnyRef linking (SI-5780)
- added tooltips to implicit conversions in diagrams
- fixed the intermittent dot error where node images would be left out
(dot is not reliable at all -- with all the mechanisms in place to fail
gracefully, we still get dot errors crawling their way into diagrams -
and that usually means no diagram generated, which is the most
appropriate way to fail, I think...)
@scala-jenkins
Copy link

Started jenkins job pr-scala-testsuite-linux-opt at https://scala-webapps.epfl.ch/jenkins/job/pr-scala-testsuite-linux-opt/401/

@scala-jenkins
Copy link

jenkins job pr-scala-testsuite-linux-opt: Success - https://scala-webapps.epfl.ch/jenkins/job/pr-scala-testsuite-linux-opt/401/

@ghost ghost assigned heathermiller Jul 3, 2012
Copy link
Member

Choose a reason for hiding this comment

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

Note to self: we should change this to a modal box with links instead.

@heathermiller
Copy link
Member

Thanks a lot Vlad! LGTM
We still have TODOs before 2.10, mostly regarding usability, but this looks really excellent! Nice going on better-organizing the model, the way you organized it makes a lot of sense.

@heathermiller
Copy link
Member

Out of curiosity, could you point me to what you did to solve ticket SI-5212? (I ask because if there's an automatic js redirect, one could wind up in a mess where there are iframes embedded within iframes embeded within iframes, etc...)

@heathermiller
Copy link
Member

Also, sure, the raw output looks like a more desirable way to test. However, could you clarify what workflow-change that you mean in the commit message (in 44ec110) when you say,

"There is a cool project by Stefan Zeiger to export
the scaladoc model in JSON, but with the language and scaladoc being so
quick to evolve, it'll be a pain to properly maintain. In the long-run,
the plan is to sample a couple of raw files on each build and email me
the diff. If I spot anything that may be wrong I can fix it, revert the
commit or at least file a bug."

@VladUreche
Copy link
Contributor Author

Regarding ticket SI-5212: Damien fixed it, the change is here: https://github.com/damienobrist/scala/commit/5efb003605bbe86d91a424e96b88cbe901655f4e

@VladUreche
Copy link
Contributor Author

Raw output has 2 main drawbacks:

  • it depends on the html page that gets generated, so any layout change significantly changes the raw output (remember the HTML tests? They test the raw output, and any layout change breaks them)
  • it won't capture all the aspects of the model, such as the signatures that entities receive or the diagram contents. (as they're embedded in html elements)

On the other hand, hard-coding an export in JSON is not going to work well either, as there's no automated way of outputting JSON from the entities -- and doing it by hand is error-prone and hard to maintain.

All in all, outputting the raw pages helps a lot when you make model changes without touching the layout -- you'll easily spot a missing/extra member or a missing class, which otherwise would have gone unnoticed, but unfortunately that's the only scenario where it's really useful.

@heathermiller
Copy link
Member

That wasn't the question I asked. I asked about the workflow. You're proposing ambiguous changes to the workflow-- you want to "sample raw files" and hope that people will manually email you diffs? Sounds like it wouldn't really work in practice if arbitrary people (in LAMP/Typesafe or the community) are supposed to know that they're supposed to be personally emailing you diffs from time to time on some arbitrary sampling of files. And it doesn't sound very thorough if you expect sampling a few files to find errors for you. (Think about the errors you found which lead to you wanting to add DocTemplateImpl, NoDocTemplateMemberImpl and NoDocTemplateImpl. There were only a handful of places where some synthetically-generated entity didn't have a page that should've had one, right? I don't think that sampling a small number of files would have ever enabled you to find these.)

It's also just wrong, in the interest of a productive and healthy workflow, to have to update a bunch of failing tests each time you make a trivial change to anything in scaladoc.

That said, I'm still fully against scaladoc tests, and I don't think this solution really solves much (it helps in probably 10% of cases, max). It's also not what we all discussed and agreed to a while back. I still think that the way tests are handled doesn't make sense at all-- this is evident in the somewhat accepted behavior of just disabling or commenting-out failing scaladoc tests when they pop up.

You also didn't explain how you believe JSON is error prone. I don't agree at all (you stated it twice, but didn't validate why you think it's more error-prone than plain text). If you're saying that the language is changing too fast, and that we might add a keyword or something that would require someone to have to change the JSON parser in scaladoc, then that's not a very strong argument. This is actually a scenario where it's not totally blasphemous to manually update three dozen failing tests by hand (rather than how we currently do it-- manually update three dozen failing tests by hand each time you touch scaladoc).

All in all, outputting the raw pages helps a lot when you make model changes without touching the layout -- you'll easily spot a missing/extra member or a missing class, which otherwise would have gone unnoticed, but unfortunately that's the only scenario where it's really useful.

How realistic is this to have to run every night and on every check-in build? It sounds worthwhile only when you, Vlad, are fixing the model (as I'm unaware of anyone else these days who touches the model). It sounds like a really bad idea to run these so often, though, as you're essentially forcing everyone who might touch scaladoc to have to deal with tests that are all-too-sensitive about anything else except for the model that only you touch. That doesn't make sense. This makes sense for manual testing (you on your own), but not nightly and frequent automatic tests.

You're shooting yourself in the foot if you want scaladoc contributors from the community. Why? 95% of people contributing to scaladoc will end up having to touch the front-end, where you'll force people to have to deal with a billion inflexible tests failing.

@VladUreche
Copy link
Contributor Author

All I'm saying is I want a separate jenkins job that periodically generates the raw output, does a diff for a couple of important pages (like List) and emails it to me. It won't fail any tests on anyone, it will just enable me to check that the model changes didn't break anything. One example: when Simon added backticks everywhere, he completely broke the usecase types and linking -- if I could have seen the extent of the damage I could have reacted quicker to either adapt the model or revert his commit. But if I can't get a bird's eye view of what changed you can't expect me to properly fix it, no? And for you not to worry about it: If layout changes between two job runs, I can just ignore the diff, as long as no model changes took place.

Regarding JSON -- how would you do it? Would you use an automated tool based on reflection, or would you code it up yourself? If (1), you'd be exposing internal fields of the _Impl classes, that's not what you want to test. If (2), you'd need to keep the export code in sync with the model _by hand*, and that's error prone -- who can check you exported all the fields correctly?

Regarding commenting out scaladoc tests:

  • the html tests will be gone at some point, I just don't want to spend two days on rewriting them now (If you have such strong feelings about them, why not spend some time translating a couple of them to model tests, I did that whenever I had failing html tests)
  • commenting out a test, either in scaladoc or anywhere else should not be done. That's not to say it hasn't been done before, because people just don't want to spend the time figuring out what's failing, but that's not something we should accept, especially since we have the pull request mechanism.

@VladUreche
Copy link
Contributor Author

A note to scala gatekeepers (@adriaanm @jsuereth and @paulp, I guess) -- this pull request got it's confirmation from Heather and we should really include it in M5. So please MergeThxBye this patch before M5!
KThxBye!

jsuereth added a commit that referenced this pull request Jul 7, 2012
@jsuereth jsuereth merged commit 7ca925e into scala:master Jul 7, 2012
VladUreche added a commit to VladUreche/scala that referenced this pull request Jul 16, 2012
As suggested by Heather in pull request scala#816.
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.

5 participants