Skip to content
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.

Sign up for GitHub

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

Jump to bottom

KotlinX.Coroutines 1.4.1 #1050

Closed
wants to merge 23 commits into from
Closed

KotlinX.Coroutines 1.4.1 #1050

wants to merge 23 commits into from

Conversation

antonioseric
Copy link
Contributor

No description provided.

@antonioser
Copy link
Contributor

waiting #987 to be merged

@moljac moljac mentioned this pull request Dec 12, 2020
Merged
@moljac
Copy link
Member

moljac commented Dec 12, 2020

Must check possible duplicate:

#1053

moljac
moljac requested changes Dec 20, 2020
View reviewed changes
Copy link
Member

@moljac moljac left a comment

Choose a reason for hiding this comment

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

It would be good to prefix nugets/namespaces/assemblies with Xamarin if author is Microsoft.

@moljac
Copy link
Member

moljac commented Dec 20, 2020 •
edited

@antonioseric @antonioser

Antonio

I had to bind really fast 2 new bindings for 4-5 new AndroidX artifacts (androidx.window and androidx.work groups)

#1053

Would it be possible to add remaining stuff to Kotlin folder? IMO this will be most likely updated all together (though not sure)

@antonioseric
Copy link
Contributor Author

I prefer not to put in Kotlin folder KotlinX is released separately. I think it is better to be decoupled. I change rootnamespace and binding namespace.

Currently working on sample app (plan to finish it today)

@moljac
Copy link
Member

moljac commented Dec 20, 2020

Don't forget nuget packagenames prefix please

@moljac
Copy link
Member

moljac commented Dec 20, 2020

OK. I will publish 1.3.4 from other PR and then delete it afterwards.

@antonioseric antonioseric marked this pull request as ready for review December 20, 2020 22:39
@4brunu
Copy link
Contributor

4brunu commented Mar 3, 2021

Any news on this? 😊

@antonioseric antonioseric mentioned this pull request Mar 5, 2021
Closed
@antonioseric
Copy link
Contributor Author

@moljac can we merge this and publish nugets?

@4brunu
Copy link
Contributor

4brunu commented Apr 3, 2021

Any news on this?
@moljac @jpobst @mattleibow

@moljac moljac requested a review from jpobst May 7, 2021 12:14
jpobst
jpobst reviewed May 7, 2021
View reviewed changes
Copy link
Collaborator

@jpobst jpobst left a comment

Choose a reason for hiding this comment

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

Namespace that is not renamed or hidden:
### New Namespace Kotlin.Coroutines.Jvm.Internal

Android/KotlinxCoroutines/source/Kotlinx.Coroutines.Android/Transforms/Metadata.xml Outdated
@@ -0,0 +1,6 @@
<metadata>

<attr path="/api/package[@name='kotlinx.coroutines.android']" name="managedName">Xamarin.KotlinX.Coroutines.CoroutinesAndroid</attr>
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think we need to pick a standard capitalization for Kotlinx/KotlinX and be consistent with it.

Copy link
Member

Choose a reason for hiding this comment

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

Changing all to KotlinX

https://www.nuget.org/packages?q=Kotlinx

Copy link
Collaborator

Choose a reason for hiding this comment

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

Looks like our existing packages have already committed to KotlinX:
https://www.fuget.org/packages/Xamarin.KotlinX.Coroutines.Core/1.3.4

😉

Android/KotlinxCoroutines/source/Kotlinx.Coroutines.Core.Jvm/Kotlinx.Coroutines.Core.Jvm.csproj Outdated
@@ -0,0 +1,55 @@
<?xml version="1.0" encoding="utf-8"?>
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't think we want to bind these libraries that aren't intended for Android:

  • Xamarin.Kotlinx.Coroutines.Core.Jvm
  • Xamarin.Kotlinx.Coroutines.Core.Jdk8
  • Xamarin.Kotlinx.Coroutines.Core.Reactive
  • Xamarin.Kotlinx.Coroutines.Core.Rx2

Today it looks like we only publish 2 packages:

  • Xamarin.KotlinX.Coroutines.Core
  • Xamarin.KotlinX.Coroutines.CoroutinesAndroid

https://www.nuget.org/packages?q=xamarin+kotlin+coroutine

Copy link
Member

Choose a reason for hiding this comment

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

Today it looks like we only publish 2 packages:

I think I bound those from Kotlin folder, just minimal dependencies I needed for GPS-FB-MLKit.

So, deleting or not publishing other packages?

Copy link
Member

Choose a reason for hiding this comment

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

Dependencies listed:

  • kotlinx-coroutines-android

    • dependencies

      • kotlin-stdlib

      • kotlinx-coroutines-core-jvm

  • kotlinx-coroutines-core-jvm

    • dependencies

      • kotlin-stdlib

      • kotlin-stdlib-common

  • kotlinx-coroutines-jdk8

    • dependencies

      • kotlin-stdlib

      • kotlinx-coroutines-core-jvm

  • kotlinx-coroutines-reactive

    • dependencies

      • kotlin-stdlib

      • kotlinx-coroutines-core-jvm

      • org.reactivestreams:reactive-streams

  • kotlinx-coroutines-rx2

    • dependencies

      • kotlin-stdlib

      • kotlinx-coroutines-core-jvm

      • io.reactivex.rxjava2:rxjava

      • kotlinx-coroutines-reactive

Already bound:

  • Kotlin.StdLib
  • Xamarin.Android.ReactiveStreams
  • RxJava

So seem like those could be used on Android and it

@moljac moljac changed the title Kotlinx.Coroutines 1.4.1 KotlinX.Coroutines 1.4.1 May 11, 2021
@moljac
Copy link
Member

moljac commented May 11, 2021

because of conflicts manual merge had to be performed.

branch: https://github.com/xamarin/XamarinComponents/tree/antonioseric-KotlinxCoroutines-1.4.1

PR: #1155

@moljac moljac self-assigned this May 13, 2021
@moljac moljac added do-not-merge in-progress Work is in progress labels May 13, 2021
@moljac moljac mentioned this pull request May 13, 2021
Merged
@moljac
Copy link
Member

moljac commented May 13, 2021

closing this PR. Published from other PR/branch after manual merging

@moljac moljac closed this May 13, 2021
@antonioseric antonioseric deleted the KotlinxCoroutines-1.4.1 branch July 17, 2021 11:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jpobst jpobst jpobst left review comments

@moljac moljac moljac approved these changes

Assignees

@moljac moljac

Labels
do-not-merge in-progress Work is in progress
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@antonioseric @antonioser @moljac @4brunu @jpobst