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

Allow custom sources for Javadoc and Scaladoc #1385

Merged
merged 16 commits into from
Jul 14, 2021
Merged

Conversation

Iltotore
Copy link
Contributor

This pull request allows the user to define custom sources for Javadoc/Scaladoc though processedDocSources:

  • JavaModule#docJar now uses processedDocSources instead of allSources
  • ScalaModule#docJar now uses processedDocSources instead of os.walk(compile().classes.path).map(PathRef(_))

Fixes #1384 .

Copy link
Member

@lefou lefou left a comment

Choose a reason for hiding this comment

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

I'm not that happy with the name processedDocSources, as I immediately asked myself what "processed" could mean. Why not just docSources? Some scaladoc should indicate the purpose of the new target.

Also, one motivation to customize the docSources could be to adapt the set of sources files, so it might be probably worth a though, if we could feed files instead of dirs.

It looks like some unrelated formatting changes went into this PR.

scalalib/src/ScalaModule.scala Outdated Show resolved Hide resolved
@Iltotore
Copy link
Contributor Author

Iltotore commented Jun 24, 2021

It looks like some unrelated formatting changes went into this PR.

Yes, I just noticed my IDE reformatted the code. If needed, I can undo this reformat.

I'm not that happy with the name processedDocSources, as I immediately asked myself what "processed" could mean. Why not just docSources?

I also thook about docSources but this method already exists and it isn't the same feature. According to scaladocs, docSources is for static (not processed by scaladoc) files.

What if we have, for some obscure reason, a tasty file and we just want to have it in our doc without processing it ?

@lefou
Copy link
Member

lefou commented Jun 24, 2021

It looks like some unrelated formatting changes went into this PR.

Yes, I just noticed my IDE reformatted the code. If needed, I can undo this reformat.

That would be nice. If you enable scalafmt in your IDE, it should automatically use the .scalafmt.conf from the repo.

I'm not that happy with the name processedDocSources, as I immediately asked myself what "processed" could mean. Why not just docSources?

I also thook about docSources but this method already exists and it isn't the same feature. According to scaladocs, docSources is for static (not processed by scaladoc) files.

What if we have, for some obscure reason, a tasty file and we just want to have it in our doc without processing it ?

I must have overlooked the existing but quite new docSources. Unfortunately the existing docSources has also misleading name, as it does not contain sources (to be process by the doc tool) but static resources to be bundled as-is. I guess we should rename to docResources and deprecate the old name for the long-term benefit. The whole docSources seems to be unsupported outside of Scala 3, so we might as well clear it up without deprecation cycles, which also allows us to re-use it's name now.

Besides, the "processed" prefix is misleading, as these sources are going to be processed by the doc tool.

@Iltotore
Copy link
Contributor Author

Iltotore commented Jun 24, 2021

That would be nice. If you enable scalafmt in your IDE, it should automatically use the .scalafmt.conf from the repo.

That's what I did. I will push the code formatted using scalafmt

I guess we should rename to docResources

Do you want me to commit this change then, use docSources instead of processed ?

@lefou
Copy link
Member

lefou commented Jun 25, 2021

I guess we should rename to docResources

Do you want me to commit this change then, use docSources instead of processed ?

Yes, please!

To avoid conflicts with users who used that target to provide static resources, we should probable output a warning in case the outcome of docSources isn't empty but does not contain any scala or java source files. Something like: "Since mill after 0.9.8 docSources should only be used to provide source files. To provide static resources, please use docResources."

Copy link
Contributor Author

@Iltotore Iltotore left a comment

Choose a reason for hiding this comment

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

Earlier we talked about reformatting modified files using ScalaFMT but it indicated that they're already formatted correctly.

@Iltotore
Copy link
Contributor Author

Iltotore commented Jun 25, 2021

Once this change is approved, I will add the suggested warning.

@lefou
Copy link
Member

lefou commented Jun 25, 2021

Earlier we talked about reformatting modified files using ScalaFMT but it indicated that they're already formatted correctly.

Scalafmt isn't going to remove new lines which you introduced earlier.

scalalib/src/JavaModule.scala Outdated Show resolved Hide resolved
scalalib/src/ScalaModule.scala Outdated Show resolved Hide resolved
scalalib/src/ScalaModule.scala Show resolved Hide resolved
@Iltotore Iltotore requested a review from lefou June 26, 2021 10:01
scalalib/src/JavaModule.scala Outdated Show resolved Hide resolved
scalalib/src/ScalaModule.scala Outdated Show resolved Hide resolved
scalalib/src/ScalaModule.scala Show resolved Hide resolved
scalalib/src/ScalaModule.scala Outdated Show resolved Hide resolved
@Iltotore
Copy link
Contributor Author

Iltotore commented Jul 1, 2021

This last change works for Scala 3 and Scala 3 Milestone. Haven't tested with Scala 2 yet but it should as I simply applied the same changes (except for tasty files filter) and it also uses packageWithZinc. I'll test it when available.

EDIT: Seems to work in Scala 2.x.

@lefou
Copy link
Member

lefou commented Jul 8, 2021

There are still failing tests related to scaladoc.

@Iltotore
Copy link
Contributor Author

Iltotore commented Jul 8, 2021

Actually, I don't really understand where this error comes from:

-- Error: C:\Users\rapha\IdeaProjects\mill\target\workspace\mill\scalalib\DottyDocTests\eval\multiple\multidocs\compile\dest\classes\pkg\SomeClass.tasty:2:407 
2 |??u?6??p?V3☻?@???o?t☻?3???p?V2☻?‼↨ ?☺??☺???p?V3???u?3?3?q?p?3?u?Pu?Pu?3?v?\r☻?3????)3????)u?3???p?Vu?3?R3♥?♠↨???)3???p?V3?↨?????3?↨\♦♦↨????3?↨?????u?3?t♥?P3☻??p?V3?↨‼↨??3☻???p?V3☻?@??☺?????
??☺????~???????????????????????????????????????????????????????☺?????☺?????☻?}???????????????????????????????????????/** A doc comment. */▬?☻?/** A doc comment. */▬?☻?/** A doc comment. */▬?
  |

                          ^
  |                                                illegal character '\u0000'

Seems like an encoding error but if I'm not wrong, I didn't change something about file encoding.

Failing tests are related to pre-release Dotty. I'll check the code tomorrow.

EDIT: After some researches, the compiled classes already have illegal characters once compiled. I'm not sure this issue comes from this PR the master branch doesn't have this issue so it comes from my fork. I don't know if it is an old bug patched in master or if it is caused by my changes. I'll update my fork with the upstream.

@@ -185,6 +201,8 @@ trait ScalaModule extends JavaModule { outer =>
)
}

override def docSources: Sources = T.sources(compile().classes.path)
Copy link
Member

Choose a reason for hiding this comment

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

This seem not correct for early dotty / scala 3 versions (isDotty(scalaVersion()) || isScala3Milestone(scalaVersion())) which use dottydoc and consumes scala source files.
Only Scala 3 releases (isScala3(scalaVersion())) use scaladoc 3 tool, which consumes tasty files.

@Iltotore
Copy link
Contributor Author

Iltotore commented Jul 9, 2021

The last commit should fix this issue. Tests passed in local.

Copy link
Member

@lefou lefou left a comment

Choose a reason for hiding this comment

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

Your PR looks good now. There are still some newly introduced but unwanted newlines. Can you please remove these, and we are good to go.

scalalib/src/ScalaModule.scala Outdated Show resolved Hide resolved
Copy link
Member

@lefou lefou left a comment

Choose a reason for hiding this comment

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

Looks good to me. Thank you!

@lefou lefou merged commit a6eef45 into com-lihaoyi:main Jul 14, 2021
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.

Can't modify scaladoc sources.
2 participants