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

[Java.Interop.Tools.JavaSource] Remove block and inline @cref values #844

Merged
merged 1 commit into from
Nov 8, 2021

Conversation

pjcollins
Copy link
Member

@pjcollins pjcollins commented May 19, 2021

Context: #843
Context: xamarin/android-api-docs#23

A handful of "broken" Javadoc to C# Doc conversions have been disabled
for now. This will hopefully help reduce the number of new warnings
introduced by xamarin/android-api-docs#23, and
allow us to get an initial major documentation update landed in the
short term. Longer term, we will revisit and fix these Javadoc
conversion issues.

Additionally, Javadoc.cs has been removed from tools/generator as it
appeared to be an unused partial duplicate of JavadocInfo.cs.

Finally, the <tt> tag is no longer supported in our docs build:

HTML tag 'tt' isn't allowed. Replace it with approved Markdown or escape the brackets if the content is a placeholder.

It has been replaced with <code> when generating Android Doc links.

@pjcollins
Copy link
Member Author

pjcollins commented May 19, 2021

I've tested these changes against a smaller android-api-docs diff that only touches Android.App.Activity:
https://github.com/xamarin/android-api-docs/compare/pjc/activity-api30-update?w=1

Some notes about the diff and corresponding docs build:

  • The ApiSince removals for anything API level 21 and below are expected, due to: [Mono.Android] Use 'merge.SourceFile' to set 'ApiSince'. android#4671.
  • The docs build is one commit behind; @linkplain is shown in a code block instead of plain text.
  • There are no new warnings about broken <cref/> links as compared to master.
  • The <remarks/> related Enum suggestions in the docs build don't seem to be worth fixing. We could strip the copyright related <remarks/> element that we generate for Enums, however mdoc will still inject
    <remarks>To be added.<remarks/> if the element is missing on our end.

@jonpryor when you have a chance could you review (at least partially) the android-api-docs diff above to see if anything funky stands out to you that I may have missed? Also, what are your thoughts on ignoring the @exception, @throws, and @see blocks entirely for now? Maybe we're better off keeping these even with broken links and potentially skewed content?

@pjcollins pjcollins requested a review from jonpryor May 19, 2021 20:27
@jonpryor
Copy link
Member

@pjcollins wrote:

however mdoc will still inject <remarks>To be added.<remarks/> if the element is missing on our end.

This sounds like an mdoc feature request! 😄

(Or maybe even a PR? 😁)

An empty <remarks/> should prevent mdoc from inserting To be added., so when we get around to emitting enum docs -- which isn't currently the case -- then we should be able to emit an empty <remarks/>.

Which raises a related question: what does ECMA2Yaml_Enum_NoRemarks mean? Is it reported if <remarks/> exists at all? Or just if it has a non-empty value?

@jonpryor
Copy link
Member

@pjcollins asked:

when you have a chance could you review (at least partially) the android-api-docs diff above to see if anything funky stands out to you that I may have missed?

The diff is big and noisy. 😄

Thus, for "sanity", I'm hoping that viewing the HTML is sufficient, which I think would be:

Which raises a problem/conundrum: why is this page incomplete?

What I see is:

Remarks

Portions of this page are modifications based on work created and shared by the and used according to terms described in the

…and that's it: a sentence fragment!

Compare to our mdoc XML:

         <remarks>
-          <para>Portions of this page are modifications based on work created and shared by the <format type="text/html"><a href="https://developers.google.com/terms/site-policies" title="Android Open Source Project">Android Open Source Project</a></
format> and used according to terms described in the <format type="text/html"><a href="https://creativecommons.org/licenses/by/2.5/" title="Creative Commons 2.5 Attribution License">Creative Commons 2.5 Attribution License.</a></format></para>
+          <para>
+            <format type="text/html">
+              <a href="https://developer.android.com/reference/android/app/Activity#closeContextMenu()" title="Reference documentation">Java documentation for <tt>android.app.Activity.closeContextMenu()</tt>.</a>
+            </format>
+          </para>
+          <para>
+                    Portions of this page are modifications based on work created and shared by the 
+                    <format type="text/html"><a href="https://developers.google.com/terms/site-policies" title="Android Open Source Project">Android Open Source Project</a></format>
+                     and used according to terms described in the 
+                    <format type="text/html"><a href="https://creativecommons.org/licenses/by/2.5/" title="Creative Commons 2.5 Attribution License">Creative Commons 2.5 Attribution License.</a></format></para>
         </remarks>

Where is the "Java documentation for" text + link? Where is the "Creative Commons 2.5 Attribute License" link? What's going wrong there?

Compare to the "same" review page for xamarin/android-api-docs#23:

which shows:

Remarks

Java documentation for android.app.Activity.closeContextMenu().

Portions of this page are modifications based on work created and shared by the Android Open Source Project and used according to terms described in the Creative Commons 2.5 Attribution License.

Is this rendering difference caused by the PR? Caused by "underlying tooling changes" between January and now? Something else?

@jonpryor
Copy link
Member

@pjcollins: "unrelated", should we try to figure out how to make the "Portions of this page are modifications" blurb a single line instead of a 5 line monstrosity? Maybe we just need to make javadoc-copyright.xml a one-line monstrosity?

@jonpryor
Copy link
Member

jonpryor commented May 19, 2021

@pjcollins: as part of review, i fear we may have a java-source-utils bug/deficiency: we're not importing docs for Activity.finishAffinity().

Maybe this is one of the cases where we're getting an exception while translating?

Specifically, the imported <remarks/> is "empty" (only contains link to reference documentation + copyright notice), while the Javadoc contains:

Finish this activity as well as all activities immediately below it in the current task that have the same affinity. This is typically used when an application can be launched on to another task (such as from an ACTION_VIEW of a content type it understands) and the user has used the up navigation to switch out of the current task and in to its own task. In this case, if the user has navigated down into any other activities of the second application, all of those should be removed from the original task as part of the task switch.

Note that this finish does not allow you to deliver results to the previous activity, and an exception will be thrown if you are trying to do so.

(Unrelated: how nice of them to tell us what exception will be thrown if we attempt to deliver results to the previous activity…)

Ditto finishAfterTransition(), finishAndRemoveTask(), etc.

@pjcollins
Copy link
Member Author

Thanks for the review so far! Comparing my branch to yours I see no difference, I've got some more digging to do...

I did try to make javadoc-copyright.xml a single line, but it was still expanded to multiple lines when generating the javadoc. Maybe there's a formatting argument that can be used, or maybe we could touch it up after generation to improve this. Unfortunately even after manually changing the javadoc to have a single copyright line, the git diff didn't improve much due to the presence of the new java documentation link above the copyright links.

The missing finish methods are strange, none of them are mentioned explicitly in the 2058 mentions of
Unable to translate remarks for $type in a local binlog. I'll have to look into this as well.

@jonpryor
Copy link
Member

jonpryor commented May 19, 2021

@pjcollins wrote:

The missing finish methods are strange

Relatedly strange, we do have <summary/> data for those methods, so some Javadoc information was imported! Somewhat odd that we'd have <summary/>, and also that the imported <summary/> isn't also part of the imported <remarks/>, considering that <summar/> is generated by parsing the first sentence of <remarks/>!

🤔

@pjcollins
Copy link
Member Author

The <remarks> section of Android.App.Activity has not changed in this branch, yet it is also now rendering incorrectly. This leads me to believe that there may be an issue in the docs build / review display somewhere.

@jonpryor
Copy link
Member

@pjcollins: on Teams, @davidbritch wrote:

Builds are meant to be triggered when you push content to a branch, but there are all kinds of reasons why that might not happen. For peace of mind it's easier to rely on a PR to trigger a build - that way there'll be a build report attached to the PR that outlines any problems.

Thus, For Great Sanity™, your pjc/activity-api30-update branch should probably become a PR for generation purposes.

@pjcollins
Copy link
Member Author

I tried this earlier today to no avail. The "Preview URL" link in this comment shows the same behavior xamarin/android-api-docs#25 (comment). I'm trying to read up on the publishing documentation.

@pjcollins
Copy link
Member Author

we may have a java-source-utils bug/deficiency: we're not importing docs for Activity.finishAffinity().

@jonpryor I believe this is a result of us only generating documentation for intellisense+extraremarks at the moment (#774). With this setting, all of the regular <remarks> blocks are stripped and only the copyright is injected. I can revisit enabling "full" Javadoc generation in the future once I work through a majority of the (2000!) remark parsing errors we currently have.

@jonpryor
Copy link
Member

I believe this is a result of us only generating documentation for intellisense+extraremarks at the moment

Lol, yup, that would do it!

@pjcollins
Copy link
Member Author

This has been blocked on a docs tooling/generation issue for a while, there is now a bug tracking this: https://ceapex.visualstudio.com/Engineering/_workitems/edit/438758

Context: dotnet#843
Context: xamarin/android-api-docs#23

A handful of "broken" Javadoc to C# Doc conversions have been disabled
for now.  This will hopefully help reduce the number of new warnings
introduced by xamarin/android-api-docs#23, and
allow us to get an initial major documentation update landed in the
short term.  Longer term, we will revisit and fix these Javadoc
conversion issues.

Additionally, `Javadoc.cs` has been removed from tools/generator as it
appeared to be an unused partial duplicate of `JavadocInfo.cs`.

Finally, the `<tt>` tag is no longer supported in our docs build:

    HTML tag 'tt' isn't allowed. Replace it with approved Markdown or escape the brackets if the content is a placeholder.

It has been replaced with `<code>` when generating Android Doc links.
@pjcollins
Copy link
Member Author

The API 31 documentation update that was created using the changes in this PR can be found here:
xamarin/android-api-docs#26

The following issues track fixes for the pieces disabled in this PR, and other problems noticed while reviewing parts of the API 31 docs update:
#843
#905
#907

@jonpryor jonpryor merged commit 29b553a into dotnet:main Nov 8, 2021
@github-actions github-actions bot locked and limited conversation to collaborators Apr 12, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants