jpobst
commented
4 years ago @jonpryor @moljac @mattleibow @Redth Was thinking about something like this and could use some feedback from more experienced binders than myself!
Closed jpobst closed 2 years ago
jpobst
commented
4 years ago @jonpryor @moljac @mattleibow @Redth Was thinking about something like this and could use some feedback from more experienced binders than myself!
jonpryor
commented
4 years ago Note periods are not required, however they may help prevent unintentional matches:
I think "something" should be required; I can't think of any deliberate scenario where you'd want to have MicOSoft.
Thus, I think the value for //ns-replace/@value should start with either a ^ or a ., and that the element body should "start with" the empty string or ., matching @value:
^: only replace at the start of the string, e.g. <ns-replace value="^androidx">AndroidX</ns-replace>.: only replace at "package boundaries", e.g. <ns-replace value=".accessibilityservice">.AccessibilityService</ns-replace>A problem with this dichotomy is that a nested package-part of androidx won't be touched at all. I'm not sure if this is a good thing or a bad thing.
jonpryor
commented
4 years ago If we drop the string.Replace() idea and start requiring regular expressions or something more complicated, we could instead say that we only match and replace on "package-parts":
<package-part java="androidx" managed="AndroidX" />
<package-part java="accessibilityservice" managed="AccessibilityService" />
<package-part java="core" managed="Core" />
<package-part java="io" managed="IO" />
<!-- … -->By only matching complete strings at:
.; or.s; or. and end-of-stringWe can avoid the MicrOSoft replacement scenario, while getting "reasonable" replacements "everywhere":
androidx.core.accessibilityservice => AndroidX.Core.AccessibilityService
mattleibow
commented
4 years ago What if you also use a path="..." as well as a filter?
<ns-replace value="androidx." path="/package/path/to/type[something]">AndroidX.</ns-replace>@jonpryor @moljac @mattleibow @Redth Simplified the proposal based on feedback, and changed from specified with metadata to specified with MSBuild.
jonathanpeppers
commented
2 years ago If you add support for this in MSBuild, should it just be so flexible that you don't even need Metadata.xml?
For:
<attr path="/api/package[@name='androidx.core.accessibilityservice']" name="managedName">AndroidX.Core.AccessibilityService</attr>You could put:
<AndroidMetadataAttr Include="/api/package[@name='androidx.core.accessibilityservice']" ManagedName="AndroidX.Core.AccessibilityService" />Which you could also create "shorthand" for (this would be the equivalent):
<AndroidNamespaceReplacement Include="androidx.core.accessibilityservice" Replacement="AndroidX.Core.AccessibilityService" />It doesn't seem like it's worth inventing a new pattern matching syntax? Should you still use xpath as before, but we create some "shortcuts"?
The only drawback is that incremental builds might be worse when .csproj files change vs Metadata.xml. We trigger a lot of targets to rerun when $(MSBuildAllProjects) changes.
jonpryor
commented
2 years ago To address the current proposal:
The word part match can be constrained to the beginning or end of a namespace by prepending a
[or appending a], respectively.
If we need to do this, we should instead use Regular Expression characters, ^ for beginning-of-namespace, and $ for end-of-namespace. Neither ^ nor $ are valid namespace characters in C#.
<AndroidNamespaceReplacement Include='^Androidx' Replacement='Xamarin.AndroidX' />@jonathanpeppers suggested using the //attr/@path value for the %(AndroidMetadataAttr.Identity) value:
<AndroidMetadataAttr Include="/api/package[@name='androidx.core.accessibilityservice']" ManagedName="AndroidX.Core.AccessibilityService" />
The conceptual problem with this approach is that it implies an exact match when an inexact match is desirable: /api/package[@name='androidx.core.view'] looks like it should only match the androidx.core.view package, and not nested packages such as androidx.core.view.accessibility, androidx.core.view.animation, or androidx.core.view.inputmethod. If you do want "starts-with" semantics anyway, you'd have to "munge" the XPath to insert a starts-with(), which significantly complicates implementation.
Even ignoring the implementation complexities, it doesn't look like what you want: it looks like an exact match, but we want an inexact match.
I don't think using the //attr/@value value is appropriate.
jpobst
commented
2 years ago On metadata:
Correct, this feature should not be implemented as metadata, for both performance and desired behavior reasons.
However, it could be passed to generator mixed inside a metadata file, and the metadata file reader could be extended to understand it, and hand it off to the appropriate implementation code.
ie, it could look something like:
<ns-replace source='androidx' replacement='AndroidX' />Then we could extend our MSBuild support to also allow for <AndroidMetadataAttr> elements if desired.
I'm torn if that's actually a good idea or not. 😜
jpobst
commented
2 years ago If we need to do this, we should instead use Regular Expression characters, ^ for beginning-of-namespace, and $ for end-of-namespace. Neither ^ nor $ are valid namespace characters in C#.
I'm up for better ideas for than square brackets, but I'm not a fan of using RegEx characters. I do not think they have any meaning to our average target user. (I've used plenty of regular expressions in my career but did not know those (or any other regex) characters.)
My thought with square brackets was some familiarity with "ranges", even though this is clearly not the same mental construct.
NuGet versioning:
<PackageReference Include="ExamplePackage" Version="[1,3)" />C# ranges:
var range = words[6..];After a little more thought, I think maybe periods are my current preferred option:
Start of namespace:
<AndroidNamespaceReplacement Include='Androidx.' Replacement='Xamarin.AndroidX' />End of namespace:
<AndroidNamespaceReplacement Include='.Androidx' Replacement='Xamarin.AndroidX' />
moljac
commented
2 years ago @jonathanpeppers
If you add support for this in MSBuild, should it just be so flexible that you don't even need Metadata.xml?
My philosophy is not to be invasive, so I used the fact that
You will see in most of my bindings Metadata.Namespaces.xml only for package names.
I did MsBuild custom task delivered via NuGet to create such Metadata.Namespaces.xml.
and I "force" developer to verify and change more appropriate .NET namespace name:
I reused our tooling and generated intermediate files (metadata.*.xml). Didn't want to clutter MSBuild. Mostly because
of my MSBuild skills and it was contrary to SDK style AKA minimal style csproject concept.
This improves bindings productivity a lot, by not needing to collect nodes from api.xml.
The general idea was:
PrivateAssets="all") and Similar nuget was for decompiling support (javap, procyon, cfr, smali, krakatau...). Add nuget, build, analyse files, fix metadata, remove nuget.
Focusing on RegEx imposes requirement that the binder is RegEx expert. Reducing number of potential users (binders). Anything complex will result in expectations that we should do it.
The most time consuming steps are
api.xml for given XPath expression (even better open editor code -g path/api.xml:$LINE_NUMBER)here come list of my planned nice-to-haves...
jonpryor
commented
2 years ago @jpobst wrote:
I'm not a fan of using RegEx characters. I do not think they have any meaning to our average target user.
Fair.
After a little more thought, I think maybe periods are my current preferred option:
Start of namespace:
<AndroidNamespaceReplacement Include='Androidx.' Replacement='Xamarin.AndroidX' />End of namespace:
<AndroidNamespaceReplacement Include='.Androidx' Replacement='Xamarin.AndroidX' />
I like this idea.
jonpryor
commented
2 years ago @jpobst wrote:
this feature should not be implemented as metadata … However, it could be passed to
generatormixed inside a metadata file
This is almost contradictory? What is the meaning of the word "metadata" if it doesn't mean "stuff within Metadata.xml"?
So… back to the beginning. What is the intent? To simplify package renames, in particular so that nested package names are automatically renamed when a parent package name is changed.
What's the benefit of doing this as an <ItemGroup/> vs. within Metadata.xml? Why prefer one over the other?
Benefits to <ItemGroup/> include:
generator et. al get so good that we don't need Metadata.xml anymore, then we're future proof! (lol?)@(AndroidNamespaceReplacement) can be tossed into a .projitems file, included into NuGet packages, and projects referencing those NuGet packages can automatically obtain those replacements.
Not sure if this is actually a good idea. It sounds cool, certainly, but is it really?
Regardless, item groups lends themselves to this form of (ab)use.
Benefits to Metadata.xml include:
Metadata.xml anyway.<ns-replace/> across projects. generator --fixup can be specified multiple times, but it's not typically a common use case. (Though @(TransformFile) could be provided via NuGet package, so this wouldn't be significantly harder, just "different": the NuGet package would need to include a Metadata.xml file and add it to @(TransformFile).)Include as an adjective.If we thought a "Metadata-less binding project" was possible in the "near-term", I'd be personally more inclined toward an item group. Otherwise, I'm more inclined to stick it into Metadata.xml.
jonpryor
commented
2 years ago @moljac: i'm not sure if you're expressing support for using an MSBuild item group or for using Metadata.xml…. Please clarify?
moljac
commented
2 years ago i'm not sure if you're expressing support for using an MSBuild item group or for using Metadata.xml…. Please clarify?
TL&DR: expressing support for Metadata.xml (being in favor of)
I mentioned MsBuild combined with NuGet as sample od delivering/shipping new features (improvements) and being minimally intrusive. Drawback could be inner-loop performance (nuget downloads, restore, performance hits for MSBuild custom tasks)
jpobst
commented
2 years ago this feature should not be implemented as metadata … However, it could be passed to generator mixed inside a metadata file
This is almost contradictory? What is the meaning of the word "metadata" if it doesn't mean "stuff within Metadata.xml"?
Yeah, that wasn't worded real well, and requires knowledge of how our metadata system works.
I meant it will not use the current system of using XPath to manipulate an XML representation of an api (api.xml).
It will instead be applied via a new, different processor built just for this purpose. However, the user-specified configuration for this process could be given via a metadata.xml file.
Today, when
generatorcreates a .NETnamespacefrom a Javapackage, it applies a simple pascal case transformation to make the namespace match established .NET naming standards.For example:
package android.databasebecomesnamespace Android.Database.However there are a few scenarios that this is not a good fit for.
(1) Words where Pascal case is not desired:
androidx->AndroidX(2) Java package names are often longer than C# namespaces:com.google.android.material.animation->Google.Android.Material.Animation.Both of these scenarios can lead to many repeated
metadatalines to fix:(Source)
Proposal
To help these scenarios, we could introduce a new MSBuild item that would allow simple replacements. Using MSBuild gives users the flexibility of MSBuild like conditionally specifying replacements based on target framework or importing it to multiple projects via
Directory.Build.targets. Additionally it might spare some users from the complexity ofmetadata.Examples:
Implementation Notes
These replacements would only be run for
<package>elements that do not specify a@managedNameattribute. If you use@managedNameyou are opting to provide the exact name, we will not process it further.Unlike unused metadata, these replacement will not raise a warning if they are unused.
Case Sensitivity
Replacements take place after the automatic Pascal case transform, but the compare is case-insensitive.
Thus, both of the following are equivalent:
Word Bounds
Replacements take place only on full words (namespace parts).
Thus,
Matches matches
Com.Google.Library, but notCommon.Google.LibraryorGoogle.Imaging.Dicom.Multiple full words can be used:
Word Position
The word part match can be constrained to the beginning or end of a namespace by appending a
.or prepending a., respectively.matches
Androidx.Core, but notSquare.OkHttp.Androidx.Similarly,
matches
Google.AndroidX.Compose, but notGoogle.Compose.Writer.Replacement Order
Replacements run in the order specified by the
<ItemGroup>, however adding to this group at different times may result in an unintended order.Replacements are run sequentially, and multiple replacements may affect a single namespace.
changes
Androidx.ViewtoXamarin.AndroidX.Views.