Skip to content

Coroutines support#57

Merged
tmaxxdd merged 11 commits into
release/1.0.0from
feature/coroutines-support
Nov 1, 2024
Merged

Coroutines support#57
tmaxxdd merged 11 commits into
release/1.0.0from
feature/coroutines-support

Conversation

@tmaxxdd
Copy link
Copy Markdown
Member

@tmaxxdd tmaxxdd commented Sep 22, 2024

No description provided.

@tmaxxdd tmaxxdd force-pushed the feature/coroutines-support branch from 3a29f20 to ed1d2b7 Compare September 22, 2024 10:02
@tmaxxdd
Copy link
Copy Markdown
Member Author

tmaxxdd commented Sep 22, 2024

@Nek-12 Maybe you would like to have a look on this work. I worked with coroutines some time ago

Comment thread src/commonMain/kotlin/dev/snipme/highlights/Highlights.kt Outdated
Comment thread src/commonMain/kotlin/dev/snipme/highlights/Highlights.kt Outdated
var snapshot: CodeSnapshot? = null
private set

var analysisJob: Job? = null
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This assignment is not thread safe and can leak, I'd recommend to avoid storing a job. See my comment below for a suggestion.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I see, this is probably to change

@Nek-12
Copy link
Copy Markdown
Contributor

Nek-12 commented Sep 22, 2024

Want me to propose another variant of the API?

@tmaxxdd
Copy link
Copy Markdown
Member Author

tmaxxdd commented Sep 22, 2024

Want me to propose another variant of the API?

Yes please

@tmaxxdd tmaxxdd force-pushed the feature/coroutines-support branch from ed1d2b7 to 8ddbf42 Compare September 24, 2024 19:21
@Nek-12
Copy link
Copy Markdown
Contributor

Nek-12 commented Sep 26, 2024

You are making a lot of changes, should I still work on this?

Comment thread src/commonMain/kotlin/dev/snipme/highlights/Highlights.kt Outdated
@tmaxxdd
Copy link
Copy Markdown
Member Author

tmaxxdd commented Sep 26, 2024

I have applied some of your suggestions and only changed small things like unit tests.

I don't know if you have branched from commit, copied code or created new branch from scratch.

I am experimenting and testing this code with Dart and Swift.

If you plan to make big changes in API create a separate PR even if this is this branch based

@tmaxxdd tmaxxdd self-assigned this Oct 5, 2024
@tmaxxdd tmaxxdd added this to the 1.0.0 milestone Oct 5, 2024
@tmaxxdd
Copy link
Copy Markdown
Member Author

tmaxxdd commented Oct 6, 2024

@Nek-12 Hi, did you manage to create any POC? :)

@Nek-12
Copy link
Copy Markdown
Contributor

Nek-12 commented Oct 8, 2024

Hi, I assumed that since you are making changes I should wait 😅. Can you help me understand how you want that to be done? Should I fork and open another PR into release/1.0.0 or into this branch?

@tmaxxdd
Copy link
Copy Markdown
Member Author

tmaxxdd commented Oct 8, 2024

Okay 😄

I tried creating a public API with Kotlin Flow but this works best when client side also implements Flow library and this is not my intention.

Nevertheless I am very happy to see your approach. Please create a PR pointing to release/1.0.0 with your idea. Grab the enough amount of code from me or write from scratch ;)

@tmaxxdd tmaxxdd merged commit 58401f1 into release/1.0.0 Nov 1, 2024
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