Skip to content

ONGOING / [api-xml-adjuster] add external extensibility feature and "source identifier #110

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

Closed

Conversation

atsushieno
Copy link
Contributor

This extensibility and SourceIdentifier extension can be used by external
API toolchain later.

For source cleanliness, extensibility support is done in partial methods.

…ntifier"

This extensibility and SourceIdentifier extension can be used by external
API toolchain later.

For source cleanliness, extensibility support is done in partial methods.
@atsushieno
Copy link
Contributor Author

@atsushieno atsushieno changed the title [api-xml-adjuster] add external extensibility feature and "source identifier ONGOING / [api-xml-adjuster] add external extensibility feature and "source identifier Dec 14, 2016
@atsushieno
Copy link
Contributor Author

Declaring as ONGOING as I likely need more changes to it.

@jonpryor
Copy link
Contributor

I'm actually not sure how to react to this PR. :-/

There are two closely intertwined questions here:

  1. Can xamarin-android-apitools work without this change in place? That is, is this PR, or anything resembling it, needed for xamarin-android-apitools to work?

    I don't know.

  2. Assuming that (1) is "Yes", is this approach this best one?

    I don't know.

In large part, the problem is that I don't understand -- and haven't spent the time to try to understand -- xamarin-android-apitools. The "meat" of the PR seems to be JavaApi and the JavaApi.NewExtensibleCreated and JavaApi.ExistingExtensibleFoundOnLoad, but I don't understand why these are events, or need to be events, or how these events fit together, or what other mechanism might be appropriate.

I don't know what to make of these events. (Related: I'm not personally fond of events, as it makes it hard to get the appropriate mental graph of all the interrelations, and to some extent that's by design, and can be a good and useful thing, but it also hinders understanding.)

Then there's JavaApi.GetExtension<T>() and the corresponding JavaApi.extensions dictionary. I don't immediately see why this needs to be in JavaApi; couldn't this be maintained within xamarin-android-apitools, with a ConditionalWeakTable?

If not, perhaps it could be modeled on XElement.AddAnnotation()? I suppose to some extent it is modeled on it -- JavaApi.GetExtension<T>() :: XObject.Annotation<T>() -- but similar names might be appropriate.

@atsushieno
Copy link
Contributor Author

The overall extension design is almost just a copy of the same concept in WCF.

@atsushieno
Copy link
Contributor Author

Why is XElement here while there is no XElement exposure?

@atsushieno
Copy link
Contributor Author

atsushieno/xamarin-android-apitools is there because it is super annoying to write within java.interop or xamarin-android because I always have to ask for commit approval. And that model totally kills this kind of experimental tool development.

@atsushieno
Copy link
Contributor Author

Having all those in mind:

Can xamarin-android-apitools work without this change in place?

No, you should merge the final PR.

Assuming that (1) is "Yes", is this approach this best one?

The assumption is wrong. Yes this is the best approach.

@jonpryor
Copy link
Contributor

Why is XElement here while there is no XElement exposure?

I said modeled on XElement. It's common practice when writing new APIs to make them similar to existing APIs that developers may be familiar with. This increases API consistency and makes things "familiar."

@atsushieno
Copy link
Contributor Author

The existing API is NOT XElement based and there is NO POINT of making such changes on that existing code.

@atsushieno
Copy link
Contributor Author

I don't want to be blocked by this pending PR anymore.

@atsushieno atsushieno closed this Dec 26, 2016
jonpryor pushed a commit that referenced this pull request Mar 8, 2021
Changes: dotnet/android-tools@479931c...554d45a

  * dotnet/android-tools@554d45a: [Xamarin.Android.Tools.AndroidSdk] Fix CS8600 in AndroidSdkBase (#107)
  * dotnet/android-tools@19454f9: Bump to xamarin/LibZipSharp/main@86f8ae57 [1.0.24] (#111)
  * dotnet/android-tools@3582b39: [macOS] fix DirectoryNotFoundException on clean systems (#110)
  * dotnet/android-tools@ca820e5: Bump to xamarin/LibZipSharp/main@521b54ec [1.0.23] (#109)
@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