Skip to content

Conversation

@bengavin
Copy link

@bengavin bengavin commented Jun 6, 2024

Update the MLKit.Core and MLKit.BarcodeScanning components to the latest released Google MLKit iOS SDK version

@AdamEssenmacher AdamEssenmacher self-assigned this Jun 6, 2024
@AdamEssenmacher AdamEssenmacher self-requested a review June 6, 2024 18:13
Copy link
Owner

@AdamEssenmacher AdamEssenmacher left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@bengavin can you give me permission to update this branch?

Or else please remove all of the unnecessary white space changes :)

@bengavin
Copy link
Author

bengavin commented Jun 6, 2024

... I wondered if that would come back to haunt me... who the heck designed these files anyway ;)

@AdamEssenmacher - do you have a particular editor you're using to avoid things auto-formatting on you, I'm trying to figure out what settings were used in the first place so the files don't get all munged around for small changes.

@AdamEssenmacher
Copy link
Owner

'designed' is a really generous term 🙃

Rider is my daily driver. It doesn't reformat code unless I ask it to. I'd be super open to doing a formatting pass with stylecop or whatever, but would just rather do that work in an isolated PR so we can stay focused on what's actually changed. Like, I think the changed ApiDefinition.cs files here were entirely whitespace changes and didn't actually need to be involved in the PR.

I've reverted all the whitespace changes already locally; I just don't have permission to update the branch.

Other than that, this looks like really good work and I'm super happy to be able to bring it into the project. Microsoft never even got as far as publishing these MLKit packages, and I'm certain there will be some happy developers out there!

@bengavin
Copy link
Author

bengavin commented Jun 6, 2024

Well, I think I've invited you to the repo with write access, I'm more than happy to take those whitespace changes :)

@bengavin
Copy link
Author

bengavin commented Jun 6, 2024

One thing that's ... strange, if we ever get around to including the MLKit Vision stuff, I'm honestly not sure that there's any content, since the baseline vision requirements seem to be baked into MLKit.Core.

@AdamEssenmacher
Copy link
Owner

Alright @bengavin if you want to give me a 👍 on my updates to make sure I didn't accidentally strip away anything important, I'll push the squash button and start running builds to publish the MLKit stuff to Nuget!

I'll probably publish them first with an -alpha prefix until you can validate them.

@bengavin
Copy link
Author

bengavin commented Jun 6, 2024

Looks good, just made one small tweak to remove a duplicate in the MLKit Core.targets file

@AdamEssenmacher AdamEssenmacher merged commit 2dfad6e into AdamEssenmacher:main Jun 6, 2024
@AdamEssenmacher
Copy link
Owner

Good catch.

Feel free to remove me from CoreBTS. Unless you want to put me on payroll 😂

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants