[class-parse] add support for external parameter name descriptor files. #200

atsushieno · 2017-10-26T12:17:23Z

The text file format is described at https://github.com/atsushieno/xamarin-android-docimporter-ng/blob/bee36c1181ac0778b63350987aba20a7c3d4410c/Xamarin.Android.Tools.JavaStubImporter/JavaApiParameterNamesXmlExporter.cs#L78 too.

jonpryor · 2017-10-26T14:36:41Z

src/Xamarin.Android.Tools.Bytecode/JavaParameterNamesLoader.cs

+		 * The Text Format is:
+		 * 
+		 * package {packagename}
+		 * ---------------------------------------


Why is this --- line required as part of the file format? (Especially when it's ignored by the parser?)

Only for visibility.

jonpryor · 2017-10-26T14:37:15Z

src/Xamarin.Android.Tools.Bytecode/JavaParameterNamesLoader.cs

+			string type = null;
+			var methods = new List<Method> ();
+			foreach (var l in File.ReadAllLines (path)) {
+				if (l.Trim ().Length == 0 || l.StartsWith ("--------", StringComparison.Ordinal))


Should this file format support comments, e.g. ignore all lines starting with #? I imagine that would be useful.

jonpryor · 2017-10-26T14:37:41Z

As you can generally expect me to ask...

Unit tests? :-D

jonpryor · 2017-10-26T14:40:17Z

src/Xamarin.Android.Tools.Bytecode/JavaParameterNamesLoader.cs

+
+		// from https://github.com/atsushieno/xamarin-android-docimporter-ng/blob/83ed572ebfdcec8a8becd3337943a488bf56d57d/Xamarin.Android.Tools.JavaStubImporter/JavaApiParameterNamesXmlExporter.cs#L78
+		/*
+		 * The Text Format is:


Did this format come from anywhere? Any particular reason to prefer a "baroque" plain-text format and not XML, or JSON?

It's less than half size as compared to XML, and this format is 100x more readable, nicer for text searching, than XML or JSON.

There is actually XML output support in the description generator tool (and even consumer in this patch). But unless there is any reason there is no need to switch to less readable, less searchable formats, we should prefer developer-friendly text.

Google also uses similar text format to list the stable API in android_framework_base.

Google also uses similar text format to list the stable API in android_framework_base.

Link? :-)

Are there any differences? Could/should this format be made closer/identical?

This: https://raw.githubusercontent.com/aosp-mirror/platform_frameworks_base/master/api/current.txt (remember there used to be api.xml which you guys basically took for generator.exe back in 2010?)

The format is of course not the same as the entire purpose is different.

atsushieno · 2017-10-26T14:40:23Z

Show me where you wrote corresponding unit tests to this in existing code base, then I will reference and replicate that.

jonpryor · 2017-10-26T14:41:13Z

src/Xamarin.Android.Tools.Bytecode/JavaParameterNamesLoader.cs

+		 * ---------------------------------------
+		 *   interface {interfacename}{optional_type_parameters} -or-
+		 *   class {classname}{optional_type_parameters}
+		 *     {optional_type_parameters}{methodname}({parameters})


I take it we don't care about the return type then?

If we don't care about the return type, why care about parameter types? For overload disambiguation purposes?

… line. That was a feedback from jonp dotnet/java-interop#200 (comment)

jonpryor · 2017-10-26T15:12:39Z

Show me where you wrote corresponding unit tests to this in existing code base, then I will reference and replicate that.

Lack of previous tests does not remove the need to have tests for new code, as often as we might overlook that.

Show me where you wrote corresponding unit tests

Within src/Xamarin.Android.Tools.Bytecode:

Plus the original unit tests:

https://github.com/xamarin/monodroid/commit/28698eb4bdf858b51194bedf6ef3e9ce502923a0

Xamarin.Android.Tools.Bytecode is one of the more heavily unit-tested assemblies here!

Outside of src/Xamarin.Android.Tools.Bytecode...

Plus the generator tests in make run-test-generator-core.

I do not like the insinuation that I'm not writing unit tests. I may not be writing enough, but I most certainly am writing many. (The entire reason I did PR #185 was to ensure that unit tests would be written!)

atsushieno · 2017-10-26T15:35:58Z

The stability of this library is already proved by make run-api-compatibility-tests in generator, at dotnet/android#982

What will prove and improve the overall product quality is NOT tests for this particular tool but proof of stability with any changes to class-parse and generator. This will be integrated into the entire xamarin-android build system and will raise errors that generator changes you guys have been making and brings in many regressions due to lack of build integration of api-*.xml.in (that I had had to fix whenever new APIs had arrived).

This change does not impact on any other uses of class-parse. Unless you explicitly pass params.txt, the changes have no impact. Mono.Android.dll builds in xamarin-android is the ONLY use case. And what if this params.txt loader regresses? It will be immediately reported by xamarin-android builds and API compatibility tests. The results are strictly reported.

That's why testing this standalone module is almost pointless, much less important than bringing in the changes themselves.

jonpryor · 2017-10-26T18:05:08Z

tools/class-parse/Program.cs

 				{ "docspath=",
 				  "Documentation {PATH} for parameter fixup",
 				  doc => docsPaths.Add (doc) },
+				{ "parameters-description=",


Why a new class-parse --parameters-description=FILE option, instead of reusing class-parse --docspath? They're both related to parameter name fixups.

Also, PR #202 has examples of unit testing ClassParse.DocumentationPaths.

Not only because there is no reason to mess existing PUBLIC arguments with INTERNAL ONLY feature, it is NOT A DOCS PATH. Whatever feature that uses this feature for NON-docs path is WRONG.

We have allowed non-"documentation" paths to be used with class-parse --docspath since PR #97 which you -- at the time -- thought was acceptable.

The fundamental purpose to class-parse --docspath is to provide parameter name information for methods.

The fundamental purpose of this PR is to provide parameter name information for methods.

That sounds ~identical to me, and I don't see why we need a completely different abstraction in the form of JavaParameterNamesLoader when we have an existing abstraction in IAndroidDocScraper.GetParameterNames(). (Sure, the name is misleading -- "scraping" documentation isn't required -- but the semantics are the same: provide method parameter names. Rename IAndroidDocScraper to IJavaMethodParameterNameProvider and the name is no longer misleading.)

It is not preferred but acceptable. Asking for pointless change is not the same.

jonpryor · 2017-10-26T18:49:11Z

That's why testing this standalone module is almost pointless.

Assume that there's an undiscovered bug in this PR (e.g. with PR #188, which has resulted in Pos #201 and #202).

You are arguing that this is a better workflow:

Merge PR [class-parse] add support for external parameter name descriptor files. #200 (this PR).
Bump and update xamarin-android to use this new functionality.
Discover the (assumed) bug.
Have go come back to the Java.Interop repo to fix the bug from (3).

Is this likely? Hopefully not! However, at least some form of testing would at least demonstrate that the code in this repo works as intended.

atsushieno · 2017-10-27T05:32:22Z

What's absurd is that your argument doesn't take place if things are in one module. That's your problem, not mine. You could review the changes by pulling in PR in xamarin-android and its external/Java.Interop to find that everything just works, or implementing the PR builders to do so.

atsushieno · 2017-10-27T06:42:31Z

From what I observed from #202 it is not quite ready for testing that depend on ANDROID_SDK_PATH.

The text file format is described at https://github.com/atsushieno/xamarin-android-docimporter-ng/blob/bee36c1181ac0778b63350987aba20a7c3d4410c/Xamarin.Android.Tools.JavaStubImporter/JavaApiParameterNamesXmlExporter.cs#L78 too.

jonpryor · 2017-11-09T17:20:28Z

Superseded by PR #207

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

atsushieno mentioned this pull request Oct 26, 2017

No need to parse JavaDoc in class-parse anymore. dotnet/android#982

Merged

atsushieno requested review from Redth, dellis1972 and jonpryor October 26, 2017 12:20

jonpryor reviewed Oct 26, 2017

View reviewed changes

atsushieno added a commit to atsushieno/xamarin-android-docimporter-ng that referenced this pull request Oct 26, 2017

Support comment lines with # so that we don't have to specialize ----…

3b3572b

… line. That was a feedback from jonp dotnet/java-interop#200 (comment)

jonpryor reviewed Oct 26, 2017

View reviewed changes

atsushieno force-pushed the new-param-names-descriptor branch from 2d84c42 to d7dbdab Compare October 27, 2017 06:43

atsushieno force-pushed the new-param-names-descriptor branch from d7dbdab to c325f36 Compare October 30, 2017 05:46

jonpryor closed this Nov 9, 2017

github-actions bot locked and limited conversation to collaborators Apr 15, 2024

[class-parse] add support for external parameter name descriptor files. #200

[class-parse] add support for external parameter name descriptor files. #200

Uh oh!

Conversation

atsushieno commented Oct 26, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jonpryor commented Oct 26, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

atsushieno commented Oct 26, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jonpryor commented Oct 26, 2017

Uh oh!

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

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jonpryor commented Oct 26, 2017

Uh oh!

atsushieno commented Oct 27, 2017

Uh oh!

atsushieno commented Oct 27, 2017

Uh oh!

jonpryor commented Nov 9, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

atsushieno commented Oct 26, 2017 •

edited

Loading