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

Deprecate nextRunList(), new target platforms and more #20

Merged
merged 33 commits into from
Jan 9, 2024

Conversation

Myshkouski
Copy link
Contributor

@Myshkouski Myshkouski commented Jan 8, 2024

Changelog:

  1. 8b1d94f - Implement 'asIterable' method:
    nextRunList() returns List<LocalDateTime> and doesn't allow to specify datetime to start iterate with, so there is no way to get results for specific period (i.e. "return next run list for period starting from 2050-01-01T10:00:00 to 2050-01-01T11:00:00"). asIterable(start: LocalDateTime) gives some advantages:
    - now you could specify start datetime to start iterate from and collect result untill some end date (i.e. cronBuilder.asIterable(someStartDateTime).takeWhile { dateTime -> dateTime < someEndDateTime });
    - no need to store all values in a list by calling cronBuilder.asIterable(someStartDateTime).asSequence();
  2. d23fc9ce - Support linuxArm64 and macosArm64 targets

@Scogun
Copy link
Owner

Scogun commented Jan 8, 2024

Hello @Myshkouski!
Thank you a lot for the great contributing!
However, beside comments on file there are couple more:

  1. Increase version at least up to 0.9.0
  2. Update README file (old nextRunList could be removed)
  3. There should be unit tests which cover new functionality.

You can ignore Sonar warning except a one (related to this comment).
I also will check why the test action is failing.

@Scogun
Copy link
Owner

Scogun commented Jan 8, 2024

Also, should we also deprecate nextRun property? Or rename to nextRunFromNow, for example?

@Scogun
Copy link
Owner

Scogun commented Jan 8, 2024

@Myshkouski could you also change pull_request on pull_request_target here?

@Myshkouski
Copy link
Contributor Author

Myshkouski commented Jan 8, 2024

Also, should we also deprecate nextRun property? Or rename to nextRunFromNow, for example?

Changed getter implementation in 76f9e59.

If you are not actually require I'll deprecate it in next pull requests.

@Myshkouski
Copy link
Contributor Author

@Myshkouski could you also change pull_request on pull_request_target here?

Updated in 238411f

@Scogun Scogun self-requested a review January 8, 2024 22:27
Copy link
Owner

@Scogun Scogun left a comment

Choose a reason for hiding this comment

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

Waiting for build.gradle.kts revert and version and README update.

@Scogun
Copy link
Owner

Scogun commented Jan 8, 2024

So, I fixed the action.
Last steps:

  1. Revert signin.skip
  2. Add at least one unit test which will cover new functionality
  3. Increase version at least up to 0.9.0
  4. Change README

CC: @Myshkouski

@Scogun
Copy link
Owner

Scogun commented Jan 9, 2024

Remaining steps:

  1. Add at least one unit test which will cover new functionality
  2. Increase version at least up to 0.9.0
  3. Change README

CC: @Myshkouski

@Myshkouski Myshkouski changed the title Deprecate nextRunList(), gradle local config and more Deprecate nextRunList(), new target platforms and more Jan 9, 2024
@Myshkouski
Copy link
Contributor Author

Myshkouski commented Jan 9, 2024

Remaining steps:

  1. Add at least one unit test which will cover new functionality
  2. Increase version at least up to 0.9.0
  3. Change README

CC: @Myshkouski

Done:

  • Add at least one unit test which will cover new functionality
  • Increase version at least up to 0.9.0
  • Change README

@Myshkouski Myshkouski requested a review from Scogun January 9, 2024 10:30
@@ -102,6 +103,6 @@ builder.years(2021..2025)
println(builder.expression) // 0/10 5-25 5,12 ? * SUN#5 2021-2025
```
### Current status
This library is on beta version `0.8.0`.
This library is on beta version `0.9.0`.
Copy link
Owner

@Scogun Scogun Jan 9, 2024

Choose a reason for hiding this comment

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

There is also the version in this lines:

@@ -10,6 +10,7 @@ import io.kotest.matchers.shouldBe
import kotlinx.datetime.*
import kotlin.test.Test

@OptIn(DelicateIterableApi::class)
class BuilderTests {

@Test
Copy link
Owner

Choose a reason for hiding this comment

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

Also add new test where expression will start from 2050, for example, but asIterable() will be called from 2060.

@Scogun
Copy link
Owner

Scogun commented Jan 9, 2024

@Myshkouski almost there.

Copy link

sonarqubecloud bot commented Jan 9, 2024

Quality Gate Passed Quality Gate passed

The SonarCloud Quality Gate passed, but some issues were introduced.

1 New issue
0 Security Hotspots
No data about Coverage
0.0% Duplication on New Code

See analysis details on SonarCloud

@Scogun
Copy link
Owner

Scogun commented Jan 9, 2024

Approved!
However, I will release after few additional changes.

@Scogun Scogun merged commit 9c9007e into Scogun:main Jan 9, 2024
4 checks passed
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