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

Fix invalid scaladoc links which cannot be found (#353) #682

Merged
merged 1 commit into from
Sep 26, 2023
Merged

Fix invalid scaladoc links which cannot be found (#353) #682

merged 1 commit into from
Sep 26, 2023

Conversation

ydash
Copy link
Contributor

@ydash ydash commented Sep 25, 2023

Refs #353

I fixed invalid scaladoc links.

  • Add enough package name
  • Fix invalid syntax of links to a method

@nyamada-tis @ydash
Fix invalid scaladoc links which cannot be found (apache#353)
879594d 
* Add enough package name
* Fix invalid syntax of links to a method
He-Pin
He-Pin reviewed Sep 25, 2023
View reviewed changes
actor-typed/src/main/scala/org/apache/pekko/actor/typed/Scheduler.scala
@@ -45,7 +45,7 @@ trait Scheduler {
* Java API: Schedules a Runnable to be run once with a delay, i.e. a time period that
* has to pass before the runnable is executed.
*
* @throws IllegalArgumentException if the given delays exceed the maximum
* @throws java.lang.IllegalArgumentException if the given delays exceed the maximum
Copy link
Member

Choose a reason for hiding this comment

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

Maybe better just import it.

Copy link
Contributor

@pjfanning pjfanning Sep 25, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

java.lang is imported implicitly - same as the top level scala package. If scaladoc is not linking these java.lang classes unless you prefix them like this - then there seems to be a bug that we should report to scala team. I'll have to test out locally if this change actually improves the scaladoc or whether the issue is with links to Java classes generally.

Copy link
Member

Choose a reason for hiding this comment

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

Yes, if it can't lik, then its a bug of scaladoc.

Copy link
Contributor Author

@ydash ydash Sep 26, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

In my environment, linking to classes under the top level scala package is also failed without those scala. prefix:

https://github.com/apache/incubator-pekko/pull/682/files#diff-d77b49e58aab175ed0d750b8b282f3ad01daab1ba88f38b3a310dc3c2e89e179L126

He-Pin
He-Pin reviewed Sep 25, 2023
View reviewed changes
actor/src/main/java/org/apache/pekko/japi/pf/ReceiveBuilder.java
@@ -19,7 +19,7 @@

/**
* Used for building a partial function for {@link
* org.apache.pekko.actor.AbstractActor#createReceive() AbstractActor.createReceive()}.
* org.apache.pekko.actor.AbstractActor#createReceive AbstractActor.createReceive()}.
Copy link
Member

Choose a reason for hiding this comment

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

this seems wrong too.

He-Pin
He-Pin reviewed Sep 25, 2023
View reviewed changes
actor/src/main/scala/org/apache/pekko/actor/Scheduler.scala
@@ -477,7 +477,7 @@ trait Scheduler {
* Java API: Schedules a Runnable to be run once with a delay, i.e. a time period that
* has to pass before the runnable is executed.
*
* @throws IllegalArgumentException if the given delays exceed the maximum
* @throws java.lang.IllegalArgumentException if the given delays exceed the maximum
Copy link
Member

Choose a reason for hiding this comment

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

I think we can just import this , then there will be no java.lang.

@He-Pin
Copy link
Member

He-Pin commented Sep 25, 2023

@ydash thanks, this is epic, but the changeset is large than I expected, will import these classes directly reduce the lines of change?

@ydash
Copy link
Contributor Author

ydash commented Sep 26, 2023 •
edited
Loading

@ydash thanks, this is epic, but the changeset is large than I expected, will import these classes directly reduce the lines of change?

@He-Pin
Probably, it's not possible.
If you import classes which are used in only scaladoc link, following compile error is appeared:

[error] /path/actor/src/main/scala/org/apache/pekko/actor/Scheduler.scala:16:18: Unused import
[error] Applicable -Wconf / @nowarn filters for this fatal warning: msg=<part of the message>, cat=unused-imports, site=org.apache.pekko.actor
[error] import java.lang.IllegalArgumentException
[error]                  ^
[error] one error found
[error] (actor / Compile / compileIncremental) Compilation failed

@He-Pin
Copy link
Member

He-Pin commented Sep 26, 2023

@ydash I think that should be a bug in Scaladoc, because in Java, we can do it. would you like to report it scala/bug ?

@ydash
Copy link
Contributor Author

ydash commented Sep 26, 2023

@He-Pin You mean it is a bug that failures of linking to classes under java.lang package without the java.lang. prefix?

@pjfanning
Copy link
Contributor

@He-Pin You mean it is a bug that failures of linking to classes under java.lang package without the java.lang. prefix?

Yes @ydash this is what is being requested. Scaladoc should not require us to hack the scaladoc like you are doing in this PR.

pjfanning
pjfanning approved these changes Sep 26, 2023
View reviewed changes
Copy link
Contributor

@pjfanning pjfanning left a comment

Choose a reason for hiding this comment

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

lgtm - I'm willing to merge this. I must admit to not wanting to run this on my laptop first because the build takes so long. If we merge this and it proves not to work well, we can revert it.

He-Pin
He-Pin approved these changes Sep 26, 2023
View reviewed changes
Copy link
Member

@He-Pin He-Pin left a comment •
edited
Loading

Choose a reason for hiding this comment

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

lgtm, checked, we can make if simpler once scala fix this.

refs: scala/bug#12884

@He-Pin He-Pin merged commit 201992d into apache:main Sep 26, 2023
@He-Pin He-Pin mentioned this pull request Sep 26, 2023
Closed
@pjfanning
Copy link
Contributor

pjfanning commented Sep 28, 2023 •
edited
Loading

@ydash @He-Pin this change is the cause of #687 - I don't know if reverting some parts of this will be enough or if we would be better off doing a full revert and starting again.

Scaladoc has so many bugs that it may not be worth our while trying to improve it.

Edit: I'm trying a partial revert - see #688

This was referenced Sep 28, 2023
Closed
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pjfanning pjfanning pjfanning approved these changes

@He-Pin He-Pin He-Pin 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.
4 participants
@ydash @He-Pin @pjfanning @nyamada-tis