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.

Sign up for GitHub

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

Jump to bottom

Check-link-hygiene bug fix and sbt integration tweak #704

Merged
merged 5 commits into from Sep 27, 2022
Merged

Check-link-hygiene bug fix and sbt integration tweak #704

merged 5 commits into from Sep 27, 2022

Conversation

daddykotex
Copy link
Contributor

That's my bad, the check to decide whether to return exit code 1 or 0 is incorrect after my changes for the check-link-hygiene. That is: I returned Error when when there were no warnings... Found it while integrating: disneystreaming/smithy4s#472

Also, we noticed that when using --check or --check-link-hygiene from sbt, the SBT jvm will exit and it's annoying. To fix that, I introduce a SbtMain to be used when calling from a jvm.

Happy to rename SbtMain because I guess there are other tools (like mill) that would be calling that.

wdyt?

@daddykotex
Do not exit on error if that are no errors...
a1973db
@daddykotex
Do not exit when calling from sbt
5c16a81 
when using sbt and `mdoc`, if the checks fail, sbt will exit. this is
annoying and this commit is an attempt at solving this issue.

To workaround the issue, I include a second main that throws an exception
instead
@daddykotex
Copy link
Contributor Author

This time, I've actually tested the thing end-to-end (in smithy4s in that case). Also, here is how it looks:

info: Compiling 31 files to /Users/David.Francoeur/workspace/dev/smithy4s/modules/docs/target/jvm-2.13/mdoc
error: 01-overview/02-quickstart.md:10:135: Unknown link '01-overview/02-quickstart.md#for-mill2', did you mean '01-overview/02-quickstart.md#for-mill'?
This section will get you started with a simple `sbt` module that enables smithy4s code generation. For a similar setup for mill, see [Mill](#for-mill2) below.
                                                                                                                                      ^^^^^^^^^^^^^^^^^^
info: Compiled in 4.7s (1 error)
[error] [docs] java.lang.RuntimeException: mdoc failed
[error] [docs]  at scala.sys.package$.error(package.scala:27)
[error] [docs]  at mdoc.SbtMain$.main(Main.scala:23)
[error] [docs]  at mdoc.SbtMain.main(Main.scala)
[error] [docs]  at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
[error] [docs]  at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:77)
[error] [docs]  at java.base/jdk.internal.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
[error] [docs]  at java.base/java.lang.reflect.Method.invoke(Method.java:568)
[error] [docs] (docs / Compile / runMain) mdoc failed

tgodzik
tgodzik reviewed Sep 27, 2022
View reviewed changes
mdoc-sbt/src/main/scala/mdoc/MdocPlugin.scala
@@ -90,7 +90,7 @@ object MdocPlugin extends AutoPlugin {
parsed
).flatten.mkString(" ")
Def.taskDyn {
runMain.in(Compile).toTask(s" mdoc.Main $args")
runMain.in(Compile).toTask(s" mdoc.SbtMain $args")
Copy link
Contributor

Choose a reason for hiding this comment

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

Would you be able to add a scripted test to check the regression?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

sure, let me look into it

is the name SbtMain fine for you?

Copy link
Contributor

Choose a reason for hiding this comment

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

Sure, whichever is fine. Maybe it's better since it indicates that we are talking about main that should be used for a build tool.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

add scaladoc on top of it + a scripted test that checks --check and --check-link-hygiene do not exit but still fail sbt task

tgodzik
tgodzik approved these changes Sep 27, 2022
View reviewed changes
Copy link
Contributor

@tgodzik tgodzik left a comment

Choose a reason for hiding this comment

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

LGTM! Should be run another release ?

@tgodzik tgodzik merged commit 87a2df6 into scalameta:main Sep 27, 2022
@daddykotex
Copy link
Contributor Author

Ideally yes, sorry for that
The current release is not broken, but if you try to use the new --check-link-hygiene, you'll get unexpected exit(1) even if every thing is fine in your setup

@tgodzik
Copy link
Contributor

tgodzik commented Sep 27, 2022

It's running! https://github.com/scalameta/mdoc/actions/runs/3137240038

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

@kubukoz kubukoz kubukoz approved these changes

@tgodzik tgodzik tgodzik approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@daddykotex @tgodzik @kubukoz