Generator support for api.xml style java docs for parameter names #920

Redth · 2017-10-06T16:59:08Z

Adds support to binding projects to make use of ApiXmlDocPaths as a project property for including api.xml style java docs to be used by ClassParse for resolving parameter names.

This format is used by Android Support libraries (and eventually Google Play Services / Firebase).

This requires dotnet/java-interop@61c70ec

This PR supercedes and closes #916.

dnfclas · 2017-10-06T16:59:11Z

@Redth,
Thanks for having already signed the Contribution License Agreement. Your agreement was validated by .NET Foundation. We will now review your pull request.
Thanks,
.NET Foundation Pull Request Bot

jonpryor · 2017-10-30T15:05:03Z

I think this PR needs a rebase and massive cleanup. :-(

This will allow api.xml style documents to be specified as java doc inputs for the ClassParse task. Java.Interop already knows how to handle this doc source type.

Redth · 2017-10-30T15:31:03Z

@jonpryor perhaps we want to take this in a different direction altogether in light of the cleanup around DocletType in java.interop? Basically all we're doing here is concatenation all of the 'doc type paths' into one single array which gets passed over to java.interop to do the auto detection on anyways. Instead of introducing a new ApiXmlDocPaths property, should we maybe introduce a single ParamNamesDocPaths or something (i'm not married to that name) and start the process of obsoleting the other properties?

jonpryor · 2017-10-30T17:19:34Z

should we maybe introduce a single ParamNamesDocPaths or something (i'm not married to that name) and start the process of obsoleting the other properties?

Yes. We should introduce a new ClassParse.DocumentationPaths property, and update the <ClassParse/> invocation to send all the current properties to it:

    <ItemGroup>
      <_DocsPath Include="$(JavaDocPaths)" />
      <_DocsPath Include="$(Java7DocPaths)" />
      <_DocsPath Include="$(Java8DocPaths)" />
      <_DocsPath Include="$(DroidDocPaths)" />
      <_DocsPath Include="@(JavaDocJar->'$(IntermediateOutputPath)javadocs\%(FileName)')" />
      <_DocsPath Include="@(AndroidDocumentationatPath)" />
    </ItemGroup>
    <ClassParse Condition="'$(AndroidClassParser)' == 'class-parse'"
      OutputFile="$(ApiOutputFile).class-parse"
      SourceJars="@(EmbeddedJar);@(InputJar)"
      DocumentationPaths="@(_DocsPath)"
    />

Redth · 2017-10-30T19:50:22Z

@jonpryor where does <_DocsPath Include="@(AndroidDocumentationatPath)" /> come from (specifically @(AndroidDocumentationatPath) )? I'm not seeing it referenced anywhere

Redth · 2017-10-30T19:53:09Z

Also, jar2xml still appears to use the individual path types... Is that within scope of changing here, or do we just leave it as is, since it's deprecated in favour of class-parse now anyway?

jonpryor · 2017-10-30T21:41:30Z

@Redth asked:

where does @(_DocsPath) come from (specifically @(AndroidDocumentationatPath))?

It doesn't come from anywhere yet. @(_DocsPath) could go "inline" into the ExportJarToXml target, (nearly?) as-is.

@(AndroidDocumentationatPath) would be a new item group, introduced in this PR, as the "going forward" mechanism to specify documentation and/or parameter name information. It would be part of @(AvailableItemName), so that developers could add a file to their .csproj, set the Build action to AndroidDocumentationPath, and the specified file will be automagically passed to the <ClassParse/> task.

Is [jar2xml] within scope of changing here

No. It's deprecated; leave it be.

We previously had JavaDocPaths, Java7DocPaths, etc. etc which were all separate task input parameters and itemgroups, but ultimately were concatenated into one single list anyway. This merges the existing separate itemgroups into one new itemgroup and one single ClassParse task parameter.

Redth · 2017-10-31T00:51:53Z

Ahh yep, I initially misread your proposal. All makes sense, and this is what i've added.

jonpryor · 2017-10-31T01:48:51Z

Please remove the Java.Interop bump from this PR. It isn't necessary.

jonpryor · 2017-10-31T01:49:41Z

src/Xamarin.Android.Build.Tasks/Xamarin.Android.Bindings.targets

    </CreateItem>

+    <ItemGroup>
+      <_AndroidDocumentationPath Include="$(AndroidDocumentationPath)" />


I think this should be @(AndroidDocumentationPath) (item group), not $(AndroidDocumentationPath) (property).

jonpryor · 2017-10-31T01:50:48Z

This should also include:

 git diff src/Xamarin.Android.Build.Tasks/Xamarin.Android.Bindings.targets
diff --git a/src/Xamarin.Android.Build.Tasks/Xamarin.Android.Bindings.targets b/src/Xamarin.Android.Build.Tasks/Xamarin.Android.Bindings.targets
index 4016c47..6247fcf 100755
--- a/src/Xamarin.Android.Build.Tasks/Xamarin.Android.Bindings.targets
+++ b/src/Xamarin.Android.Build.Tasks/Xamarin.Android.Bindings.targets
@@ -68,6 +68,7 @@ Copyright (C) 2012 Xamarin Inc. All rights reserved.
   
   <!-- Get our Build Actions to show up in VS -->
   <ItemGroup>
+    <AvailableItemName Include="AndroidDocumentationPath" />
     <AvailableItemName Include="EmbeddedJar" />
     <AvailableItemName Include="EmbeddedNativeLibrary" />
     <AvailableItemName Include="EmbeddedReferenceJar" />

jonpryor · 2017-10-31T01:52:41Z

@atsushieno: Thoughts on the approach laid out above?

So we get the option to pick this from within the IDE.

AndroidDocumentationPath will be assumed to be an `<ItemGroup>` entry, not a `<PropertyGroup>` property unlike the other doc paths have historically been. `$` was therefore incorrect and should be `@`

atsushieno · 2017-10-31T02:41:29Z

It is a breaking change to binding developers so we can't kill those existing doc paths. JavaDocIndex is already there, so no need to add new build action. It's for mdoc-based XML docs generation, but unless you explicitly specify the project option to generate it, it won't do the job.

(And IMO we shouldn't bring in more build actions; it is already confusing developers with too many FooBarJar.)

jonpryor · 2017-10-31T16:10:05Z

It is a breaking change to binding developers so we can't kill those existing doc paths.

This doesn't "kill" those existing doc paths. They're still used, e.g. $(Java8DocPaths) is included into the @(_AndroidDocumentationPath) item group, and passed on to <ClassParse/>. Semantically, it changes nothing, because all of those separate properties were merged into a single ClassParse.DocumentationPaths anyway. It's just that now the merging is done in the .targets, not in the task.

JavaDocIndex is already there

Indeed. I'd forgotten about that. :-(

The one possible problem here is that @(JavaDocIndex) is also provided to <JarToXml/>, which doesn't (and won't!) support using api.xml-formatted XML for parameter name information. We can either ignore that, or somehow "filter" @(JavaDocIndex) to remove api.xml files.

atsushieno · 2017-11-01T02:27:06Z

API XML had never been supported in jar2xml and people should not expect it to be supported, therefore it should be just ignored.

Again, I don't like the idea of publicly accessible AndroidDocPaths build item. Not only because the reason I mentioned above (too many build actions + we already have JavaDocIndex), but also because users can never specify that target item from the IDE. Build Actions are for files, not directories on VS/VS4M.

I like the idea of incorporating JavaDocJar there, although the premise that the docs must be extracted in javadoc intermediate directory is somewhat fragile. That makes dependency tracking and future refactoring difficult and error-prone. But to avoid that we first need to kill MSBuild and reimplement everything, and it won't happen for a while, so it's inevitable.

jonpryor · 2017-11-01T14:29:42Z

API XML had never been supported in jar2xml and people should not expect it to be supported, therefore it should be just ignored.

Agreed. However, there are two ways of "ignoring":

Remove api.xml files from @(JavaDocIndex) from the set of files provided to <JarToXml/>.
Don't do anything at all. If someone adds an api.xml file to their project with a @(JavaDocIndex) build action, and also set $(AndroidClassParser)==jar2xml, they'll get some kind of "weird" error, but that's OK. (Is it OK?)

I don't like the idea of publicly accessible AndroidDocPaths build item

I previously agreed with you that we shouldn't introduce a new Build action, and we should instead reuse the existing @(JavaDocIndex) Build action for api.xml files.

Additionally, api.xml is a file. There is thus no file vs. directory problem here, and it can sanely be added and Build action changed within the IDE.

I like the idea of incorporating JavaDocJar there

Are you suggesting using a Build action of @(JavaDocJar) for api.xml files and not @(JavaDocIndex)? That's...plausible? It would complicate things, in that we currently <Unzip Sources="@(JavaDocJar)" />, but that can be filtered/corrected.

@(JavaDocIndex) feels slightly better to me.

atsushieno · 2017-11-02T09:50:32Z

Are you suggesting using a Build action of @(JavaDocJar) for api.xml files and not @(JavaDocIndex)?

No, not really, JavaDocIndex is even better. I don't like the idea of injecting that build item, but then we'd have to introduce another build action if we need this feature publicly available (do we?) If it's not for public use then I'd stick to a build property which is not exposed to users.

atsushieno · 2017-11-02T09:51:38Z

And let's stop worring about jar2xml and take option 2. I rather worry about extraneous changes to be made. Engineering cost is not free.

jonpryor · 2017-11-03T16:50:52Z

src/Xamarin.Android.Build.Tasks/Xamarin.Android.Bindings.targets


  <!-- Get our Build Actions to show up in VS -->
  <ItemGroup>
+    <AvailableItemName Include="AndroidDocumentationPath" />


As per prior comments on this thread, please remove this line.

jonpryor · 2017-11-03T16:51:12Z

src/Xamarin.Android.Build.Tasks/Xamarin.Android.Bindings.targets

    </CreateItem>

+    <ItemGroup>
+      <_AndroidDocumentationPath Include="@(AndroidDocumentationPath)" />


...and update this line to use @(JavaDocIndex).

Use JavaDocIndex instead which already exists.

dnfclas added the cla-already-signed label Oct 6, 2017

Redth added 2 commits October 30, 2017 11:24

[Xamarin.Android.Build.Tasks] Add ApiXmlPaths to ClassParse

2fa5338

This will allow api.xml style documents to be specified as java doc inputs for the ClassParse task. Java.Interop already knows how to handle this doc source type.

Bump java.interop

73598ce

Redth force-pushed the classparse-apixmldocs branch from 32cf757 to 73598ce Compare October 30, 2017 15:25

jonpryor reviewed Oct 31, 2017

View reviewed changes

jonpryor requested a review from atsushieno October 31, 2017 01:52

Redth added 3 commits October 30, 2017 22:11

Add AndroidDocumentation to AvailableItemName grp

3fb4e3d

So we get the option to pick this from within the IDE.

Fix item group include

140e187

AndroidDocumentationPath will be assumed to be an `<ItemGroup>` entry, not a `<PropertyGroup>` property unlike the other doc paths have historically been. `$` was therefore incorrect and should be `@`

Revert bump to java.interop

72d0f56

jonpryor reviewed Nov 3, 2017

View reviewed changes

Revert AndroidDocumentationPath creation

d048080

Use JavaDocIndex instead which already exists.

jonpryor merged commit f11b873 into dotnet:master Nov 3, 2017

github-actions bot locked and limited conversation to collaborators Feb 4, 2024

Generator support for api.xml style java docs for parameter names #920

Generator support for api.xml style java docs for parameter names #920

Uh oh!

Conversation

Redth commented Oct 6, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dnfclas commented Oct 6, 2017

Uh oh!

jonpryor commented Oct 30, 2017

Uh oh!

Redth commented Oct 30, 2017

Uh oh!

jonpryor commented Oct 30, 2017

Uh oh!

Redth commented Oct 30, 2017

Uh oh!

Redth commented Oct 30, 2017

Uh oh!

jonpryor commented Oct 30, 2017

Uh oh!

Redth commented Oct 31, 2017

Uh oh!

jonpryor commented Oct 31, 2017

Uh oh!

jonpryor Oct 31, 2017

Choose a reason for hiding this comment

Uh oh!

jonpryor commented Oct 31, 2017

Uh oh!

jonpryor commented Oct 31, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

atsushieno commented Oct 31, 2017

Uh oh!

jonpryor commented Oct 31, 2017

Uh oh!

atsushieno commented Nov 1, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jonpryor commented Nov 1, 2017

Uh oh!

atsushieno commented Nov 2, 2017

Uh oh!

atsushieno commented Nov 2, 2017

Uh oh!

jonpryor Nov 3, 2017

Choose a reason for hiding this comment

Uh oh!

jonpryor Nov 3, 2017

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Redth commented Oct 6, 2017 •

edited

Loading

jonpryor commented Oct 31, 2017 •

edited

Loading

atsushieno commented Nov 1, 2017 •

edited

Loading