Skip to content

Conversation

@atsushieno
Copy link
Contributor

ClassPath class had a red-herring property named "DocletType", which was
NEVER assigned and therefore never worked. Its existence is optimistic
so that it premised that any consuming code knows which doclet type its
argument javadoc is generated with.

So, just kill it and implement working code here.

Now that we get a working doclet parser, a handful of Java8 scraper issues
were discovered, so fixed them all. Namely:

  • Arrays are replaced as :A, not mere [].
    • the new DroidDoc actually uses [] as is, and I believe it used to be
      so too in Javadocs. On the other hand, [] may show up only within
      droiddocs, so skip that string.Replace() for Java8.
  • Java8 doc parsers simply passed '-' which actually needed to be escaped.
    It caused errors when argument names were simple like 'x-y' (which was
    treated as character range specification in regex).
  • regex matching for "anything except for '('" should actually be
    "... except for '(' and ')'" and then they should have been escaped too.

ClassPath class had a red-herring property named "DocletType", which was
NEVER assigned and therefore never worked. Its existence is optimistic
so that it premised that any consuming code knows which doclet type its
argument javadoc is generated with.

So, just kill it and implement working code here.

Now that we get a working doclet parser, a handful of Java8 scraper issues
were discovered, so fixed them all. Namely:

- Arrays are replaced as :A, not mere [].
  - the new DroidDoc actually uses [] as is, and I believe it used to be
    so too in Javadocs. On the other hand, [] may show up only within
    droiddocs, so skip that string.Replace() for Java8.
- Java8 doc parsers simply passed '-' which actually needed to be escaped.
  It caused errors when argument names were simple like 'x-y' (which was
  treated as character range specification in regex).
- regex matching for "anything except for '('" should actually be
  "... except for '(' and ')'" and then they should have been escaped too.
…working)

As described in the previous change, this property doesn't make sense at all,
but it is too annoying to alter existing tests as well as removing it from
class-parse.exe (Program.cs), so just make it nullable and bring back.
@jonpryor
Copy link
Contributor

So, just kill it and implement working code here.

I for one welcome our working code overlords.

That said, The Immortal Questions:

  1. Is this unit testable? If it isn't unit testable, there needs to be a good description for why.
  2. Let's see some unit tests? :-)

@atsushieno
Copy link
Contributor Author

No.

Those javadocs that are used in real productions are written in quite complicated HTML structure that I don't think there is reliable way to write by our own. And those javadocs are copyrighted. That's why it was never tested.

What we can do instead is to download those existing javadocs at test-run-time and see if it generates parameter names as expected (which by the way still does not likely: this incomplete ClassParse functionality had blocked us (me) from doing any implementation sanity check). I think such tests are going to take a lot of time (like our msbuild tests do).

And then, having this PR blocked until those lengthy testing means that our component team will never get the entire simplified binding tooling improvements JUST FOR THIS. I don't think it is welcomed situation.

@jonpryor jonpryor merged commit f291722 into dotnet:master Feb 14, 2017
jonpryor pushed a commit that referenced this pull request Oct 4, 2021
Context: https://devdiv.visualstudio.com/DevDiv/_workitems/edit/1314263
Context: c936d09

Changes: dotnet/android-tools@d92fc3e...34e98e2

  * dotnet/android-tools@34e98e2: [build] Allow Assembly "vendorization" (#136)
  * dotnet/android-tools@061bcc2: [build] Import "parent directory.override.targets" (#135)
  * dotnet/android-tools@a5194e9: [Xamarin.Android.Tools.AndroidSdk] Downgrade build-tools to API-30
  * dotnet/android-tools@440e6be: [Xamarin.Android.Tools.AndroidSdk] Update SDK component for API-31 (#134)
  * dotnet/android-tools@9b658b2: Merge pull request #133 from xamarin/ndk-r23
  * dotnet/android-tools@ff73f92: [build] Use GitInfo to generate $(Version) (#131)
  * dotnet/android-tools@4c2e36c: [Xamarin.Android.Tools.AndroidSdk] Eclipse Adoptium support (#132)
  * dotnet/android-tools@eaec4e3: [Xamarin.Android.Tools.AndroidSdk] More Microsoft Dist JDK Support (#130)
  * dotnet/android-tools@f9c1b0d: [BaseTasks] improve Task settings in AsyncTaskExtensions (#129)
  * dotnet/android-tools@efc9b67: Merge pull request #128 from dellis1972/bumpitybump
  * dotnet/android-tools@40adec0: Bump LibZipSharp and Mono.Unix to latest stable versions,
  * dotnet/android-tools@87acd6b: [Microsoft.Android.Build.BaseTasks] add StrongName  (#127)
  * dotnet/android-tools@02f7ae7: [NDK] Properly detect 64-bit NDK
  * dotnet/android-tools@623332d: Bump LibZipSharp to 2.0.0-alpha7 (#126)
  * dotnet/android-tools@c055fa8: [Microsoft.Android.Build] Bump to MSBuild 16.10.0 (#125)
  * dotnet/android-tools@49936d6: Merge pull request #124 from xamarin/update-libzipsharp
  * dotnet/android-tools@ef78dfc: Bump LibZipSharp to 2.0.0-alpha6
  * dotnet/android-tools@e3d708c: [BaseTasks] fix `\`-delimited paths on macOS (#122)
  * dotnet/android-tools@bdcf899: Reference the new Mono.Unix nuget (#123)
  * dotnet/android-tools@90d7621: [BaseTasks] add ABI detection for RIDs (#121)
  * dotnet/android-tools@79e3b97: [JdkInfo] handle invalid XML from /usr/libexec/java_home (#120)
  * dotnet/android-tools@81519fe: Add SECURITY.md (#119)
  * dotnet/android-tools@683f375: [Microsoft.Android.Build] Use MSBuild NuGets (#118)
  * dotnet/android-tools@2241489: [Xamarin.Android.Tools.AndroidSdk] Support Microsoft Dist JDK (#117)
  * dotnet/android-tools@52ef989: [Xamarin.Android.Tools.AndroidSdk] Fix CS0168 warning (#116)

Moving the Roslyn Analyzers initialization code from
`Directory.Build.props` to `Directory.Build.targets` (c936d09)
caused the Roslyn Analyzers to be applied to code imported from
`external/xamarin-android-tools`.

Previously the xamarin-android-tools code was unaffected because it
had its own `Directory.Build.props` file, so it did not traverse the
file system upwards any further to find the Java.Interop files.

Fixes for Roslyn issues has not been applied to `xamarin-android-tools`
code, so it adds ~150 warnings to the Java.Interop build.

dotnet/android-tools@ff73f925 added a
`Directory.Build.targets`, so updating `external/xamarin-android-tools`
to the latest xamarin-android-tools commit will prevent Java.Interop's
`Directory.Build.targets` file from being used, which will exclude
xamarin-android-tools from the Roslyn Analyzers.
@github-actions github-actions bot locked and limited conversation to collaborators Apr 15, 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.

3 participants