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.

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

Set of API improvements #29

Closed
wants to merge 26 commits into from
Closed

Set of API improvements #29

wants to merge 26 commits into from

Conversation

ProMix0
Copy link

@ProMix0 ProMix0 commented May 28, 2022

Closes #24
Closes #25
Closes #26
Closes #27
Closes #28

Also it unintended resolve #22

@ProMix0
Copy link
Author

ProMix0 commented May 30, 2022

#30 should help

@ProMix0
Copy link
Author

ProMix0 commented May 30, 2022

This commits will fix build Action

@ProMix0 ProMix0 mentioned this pull request May 30, 2022
Copy link
Owner

@GeeWee GeeWee left a comment

Choose a reason for hiding this comment

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

Sorry for taking my sweet time with this.
I think there's a lot of really good stuff here - and thanks for contributing ! I'm not sold on all the changes yet unfortunately, so let's iterate a few times until we get it right!

src/PeriodicTaskRunnerBackgroundService.cs Outdated Show resolved Hide resolved
src/PeriodicTaskRunnerBackgroundService.cs Show resolved Hide resolved
src/PeriodicTaskRunnerBackgroundService.cs Show resolved Hide resolved
src/CriticalBackgroundService.cs Outdated Show resolved Hide resolved
README.md Show resolved Hide resolved
@GeeWee
Copy link
Owner

GeeWee commented Jun 1, 2022

Also CI seems to be failing

@ProMix0
Copy link
Author

ProMix0 commented Jun 2, 2022

It's strange. My fork pass tests successful
image
Fork

I will try to resolve this

@ProMix0
Copy link
Author

ProMix0 commented Jun 4, 2022

I really have no idea why tests (I mean test - PeriodicTask_ShouldContinueRunningTasks_IfFailureModeIsSetToRetry()) don't passes

I guess this behavior undefined. But don't know why

@GeeWee
Copy link
Owner

GeeWee commented Jun 6, 2022

Ah, let me try to dive into the CI situation - that does seem super weird. It might take a week or so before I get around to it though.

@GeeWee
Copy link
Owner

GeeWee commented Jun 14, 2022

Turns out it's probably going to take longer - real life's getting in the way. You're not forgotten though.

@ProMix0
Copy link
Author

ProMix0 commented Jun 14, 2022

Maybe I can help?

@GeeWee
Copy link
Owner

GeeWee commented Jun 15, 2022

I mean absolutely do feel free to dive into the CI issue - there is a chance it's just a flaky test

@ProMix0
Copy link
Author

ProMix0 commented Jun 16, 2022

Can you try to run CI again? My branch passed tests three times in a row. If now problem isn't resolved... so, I will think a lot :)

@GeeWee
Copy link
Owner

GeeWee commented Jun 16, 2022

Still seems to be failing..! :/

@ProMix0
Copy link
Author

ProMix0 commented Jun 16, 2022

Finally, I found failed test
image
I guess latest commit may fix that issue

@ProMix0 ProMix0 requested a review from GeeWee June 17, 2022 06:12
Copy link
Owner

@GeeWee GeeWee left a comment

Choose a reason for hiding this comment

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

I think I have one spot that could do a little improving, but otherwise we're ready to merge this in! Thanks for sticking with me!

tests/PeriodicTasksTest.cs Outdated Show resolved Hide resolved
tests/PeriodicTasksTest.cs Outdated Show resolved Hide resolved
tests/CriticalBackgroundServiceTests.cs Outdated Show resolved Hide resolved
@GeeWee GeeWee added enhancement New feature or request minor non-breaking api changes labels Jun 19, 2022
@ProMix0
Copy link
Author

ProMix0 commented Jun 19, 2022

I rewrote tests. Yes, it's don't work again
I have a versions that it's a kind of multithread issue

So, I don't know. Tomorrow I will add loggers to tests

@GeeWee
Copy link
Owner

GeeWee commented Jul 8, 2022

After reflecting on the amount of time I have to maintain this project, I don't think I will have time to handle PR's and stuff unfortunately.

However, if you'd like to make a fork and publish that to nuget under a different name, I would be happy to link to it from this projects README

@ProMix0 ProMix0 closed this by deleting the head repository Sep 30, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request minor non-breaking api changes
Projects
None yet
2 participants