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

Fix link reference #129

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

Conversation

lqiu96
Copy link
Contributor

@lqiu96 lqiu96 commented Jun 17, 2022

Fixes #<issue_number_goes_here>

It's a good idea to open an issue first for discussion.

  • Tests pass
  • Appropriate changes to README are included in PR

@lqiu96 lqiu96 requested a review from suztomo June 17, 2022 17:45
@lqiu96 lqiu96 requested a review from a team as a code owner June 17, 2022 17:45
Copy link
Member

@suztomo suztomo left a comment

Choose a reason for hiding this comment

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

Can you create a test case that fails before the fix? It helps me to understand the root cause.

@@ -120,27 +159,43 @@ else if (uid != "")
return specList;
}

/**
* Populate the links for references based on a UID. Looker generates mappings for local context
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
* Populate the links for references based on a UID. Looker generates mappings for local context
* Populates the links for references based on a UID. Looker generates mappings for local context

@@ -28,7 +28,7 @@ items:
fullName: "com.microsoft.samples.google.SpeechSettings"
type: "Class"
package: "com.microsoft.samples.google"
summary: "Settings class to configure an instance of <xref uid=\"com.microsoft.samples.google.v1p1alpha.SpeechClient\" data-throw-if-not-resolved=\"false\">SpeechClient</xref>.\n\n <p>The default instance has everything set to sensible defaults:\n\n <ul>\n <li>The default service address (speech.googleapis.com) and default port (443) are used.\n <li>Credentials are acquired automatically through Application Default Credentials.\n <li>Retries are configured for idempotent methods but not for non-idempotent methods.\n </ul>\n\n <p>The builder of this class is recursive, so contained classes are themselves builders. When\n build() is called, the tree of builders is called to create the complete settings object.\n\n <p>For example, to set the total timeout of recognize to 30 seconds:\n\n <pre class=\"prettyprint lang-java\"><code>\n SpeechSettings.Builder speechSettingsBuilder = SpeechSettings.newBuilder();\n speechSettingsBuilder\n .recognizeSettings()\n .setRetrySettings(\n speechSettingsBuilder\n .recognizeSettings()\n .getRetrySettings()\n .toBuilder()\n .setTotalTimeout(Duration.ofSeconds(30))\n .build());\n SpeechSettings speechSettings = speechSettingsBuilder.build();\n </code></pre>"
summary: "Settings class to configure an instance of <xref uid=\"com.microsoft.samples.google.SpeechClient\" data-throw-if-not-resolved=\"false\">SpeechClient</xref>.\n\n <p>The default instance has everything set to sensible defaults:\n\n <ul>\n <li>The default service address (speech.googleapis.com) and default port (443) are used.\n <li>Credentials are acquired automatically through Application Default Credentials.\n <li>Retries are configured for idempotent methods but not for non-idempotent methods.\n </ul>\n\n <p>The builder of this class is recursive, so contained classes are themselves builders. When\n build() is called, the tree of builders is called to create the complete settings object.\n\n <p>For example, to set the total timeout of recognize to 30 seconds:\n\n <pre class=\"prettyprint lang-java\"><code>\n SpeechSettings.Builder speechSettingsBuilder = SpeechSettings.newBuilder();\n speechSettingsBuilder\n .recognizeSettings()\n .setRetrySettings(\n speechSettingsBuilder\n .recognizeSettings()\n .getRetrySettings()\n .toBuilder()\n .setTotalTimeout(Duration.ofSeconds(30))\n .build());\n SpeechSettings speechSettings = speechSettingsBuilder.build();\n </code></pre>"
Copy link
Member

Choose a reason for hiding this comment

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

The uid of com.microsoft.samples.google.SpeechClient seems incorrect in both before and after. Why?

Screen Shot 2022-06-17 at 4 24 20 PM

* Since packages (v1 and v1beta) may contain the same generated java file names, there may be some
* conflicts between which link it should be.
*
* The Looker#consumer() function guarantees that the UID (+ other combinations) will be put into the LookupContext.
Copy link
Member

Choose a reason for hiding this comment

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

What is Looker?

Comment on lines +170 to +171
* Since packages (v1 and v1beta) may contain the same generated java file names, there may be some
* conflicts between which link it should be.
Copy link
Member

Choose a reason for hiding this comment

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

Can you add why Javac doesn't have the problem of ambiguity but this tool via javadoc has trouble finding the exact class?

Comment on lines +84 to 85
static String resolveUidFromLinkContent(String linkContent, String packageName, LookupContext lookupContext) {
if (StringUtils.isBlank(linkContent)) {
Copy link
Member

Choose a reason for hiding this comment

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

Do you think it's possible not to add the additional parameter and leveraging lookupContext? For me, right now, it seems that the bug comes from lookupContext. I see the variable holds the fully-qualified class name for symbols.

Screen Shot 2022-06-17 at 4 34 17 PM

@alicejli
Copy link
Contributor

alicejli commented May 4, 2023

@lqiu96 Is this PR still needed? I'm happy to help take it on if this fix is still needed; just wanted to double check.

@lqiu96
Copy link
Contributor Author

lqiu96 commented May 4, 2023

@lqiu96 Is this PR still needed? I'm happy to help take it on if this fix is still needed; just wanted to double check.

Unfortunately, I didn't create an issue with what I was fixing (my mistake), so I forgot what this is trying to fix. IIRC (and could be wrong), this was trying to fix link references in one package (v1) from referencing another package (v1beta1) if there are duplicates. Link references should try to find the reference in the same package.

I think this is something that should be fixed eventually.

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.

3 participants