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

Feat/float next interval #213

Merged
merged 5 commits into from
Oct 2, 2024
Merged

Feat/float next interval #213

merged 5 commits into from
Oct 2, 2024

Conversation

L-M-Sherlock
Copy link
Member

@L-M-Sherlock L-M-Sherlock commented Aug 22, 2024 •
edited
Loading

@L-M-Sherlock L-M-Sherlock added the enhancement New feature or request label Aug 22, 2024
@dae
Copy link
Collaborator

dae commented Sep 10, 2024

These changes look pretty straightforward. I presume we could retain the same behavior on Anki's end, if desired, by rounding the intervals on that side? Is your intention to do that for 1+ day intervals, and treat subday intervals differently if there are no (re)learning steps?

@L-M-Sherlock
Copy link
Member Author

Yes. In the corresponding PR, I modified the code to round the interval and make sure it's greater or equal to 1 day. For interval shorter than 0.5 day, if there are no (re)learning steps, it will be converted to seconds.

@dae
Copy link
Collaborator

dae commented Sep 10, 2024

FSRS controlling subday intervals is a bit contentious, but we can discuss that in the linked PR - looks like the changes here should be ok to merge?

@L-M-Sherlock
Copy link
Member Author

It's OK to merge this PR. But it's nice if we can wait for merging #222, which doesn't need Anki side to change the code. This PR is not back-compatible, if I merge it first, I have to update a bunch of code in Anki side.

@L-M-Sherlock L-M-Sherlock marked this pull request as ready for review September 10, 2024 06:05
@L-M-Sherlock
Copy link
Member Author

I recommend merging this PR ankitects/anki#3405 first.

@L-M-Sherlock
Copy link
Member Author

@dae, do you approve this PR?

@dae
Copy link
Collaborator

dae commented Oct 2, 2024

Yes, sorry for the delay.

@L-M-Sherlock L-M-Sherlock enabled auto-merge (squash) October 2, 2024 11:06
@L-M-Sherlock L-M-Sherlock merged commit 35b80f4 into main Oct 2, 2024
3 checks passed
@L-M-Sherlock L-M-Sherlock deleted the Feat/float-next-interval branch October 2, 2024 15:36
@L-M-Sherlock L-M-Sherlock mentioned this pull request Oct 18, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dae dae dae approved these changes

Assignees
No one assigned
Labels
enhancement New feature or request
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@L-M-Sherlock @dae