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

Fix scheduling week N #92

Merged
merged 1 commit into from
Jul 2, 2024
Merged

Fix scheduling week N #92

merged 1 commit into from
Jul 2, 2024

Conversation

bastjan
Copy link
Contributor

@bastjan bastjan commented Jun 27, 2024

We incorrectly returned an error in the helper function CheckIsoWeek if ISO week N did not match current week. This PR fixes the return value to false and the error to nil as expected.

Checklist

  • Categorize the PR by setting a good title and adding one of the labels:
    bug, enhancement, documentation, change, breaking, dependency
    as they show up in the changelog
  • Update tests.

@bastjan bastjan added the bug Something isn't working label Jun 27, 2024
@bastjan bastjan requested a review from a team as a code owner June 27, 2024 14:14
We incorrectly returned an error in the helper function `CheckIsoWeek` if ISO week `N` did not match current week. This PR fixes the return value to `false` and the error to nil as expected.
@bastjan bastjan force-pushed the fix-schedule-week-n branch from e3f0246 to 1a7769b Compare June 27, 2024 14:18
Copy link

@HappyTetrahedron HappyTetrahedron left a comment

Choose a reason for hiding this comment

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

Oh wow, that was sneaky.

Would there be a way for me to cross-import this logic somehow rather than reimplement it?

@bastjan
Copy link
Contributor Author

bastjan commented Jul 2, 2024

Would there be a way for me to cross-import this logic somehow rather than reimplement it?

Yes as it is now a separate package with public interface. But:

A little copying is better than a little dependency.

I think it will be simpler long term to just copy paste.

@bastjan bastjan merged commit 0c3e232 into master Jul 2, 2024
3 checks passed
@bastjan bastjan deleted the fix-schedule-week-n branch July 2, 2024 11:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants