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/daily new limit #95

Open
wants to merge 6 commits into
base: develop
Choose a base branch
from
Open

Feat/daily new limit #95

wants to merge 6 commits into from

Conversation

cashpw
Copy link
Contributor

@cashpw cashpw commented Sep 19, 2022

This is built on top of #82. Related: #80.

@cashpw
Copy link
Contributor Author

cashpw commented Sep 19, 2022

On second thought, this could use some tests. I'll be away for the next two weeks but could add then when I get back.

l3kn
l3kn reviewed Sep 19, 2022
View reviewed changes
Copy link
Owner

@l3kn l3kn left a comment

Choose a reason for hiding this comment

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

Besides some minor nitpicks this looks good.

How set are you on limiting the number of new cards per day
instead of per review session?

org-fc-review.el Outdated Show resolved Hide resolved
org-fc-review.el Outdated Show resolved Hide resolved
org-fc-review.el Outdated Show resolved Hide resolved
@cashpw cashpw force-pushed the feat/daily-new-limit branch 2 times, most recently from 993f867 to a42c194 Compare September 19, 2022 12:33
@cashpw
Copy link
Contributor Author

cashpw commented Sep 19, 2022

How set are you on limiting the number of new cards per day instead of per review session?

I prefer daily based on my history with flashcard applications. However, this doesn't need to be the case for everyone. I've added org-fc-review-new-limit-schedule which allows users to set either 'day or 'session.

@cashpw cashpw mentioned this pull request Sep 19, 2022
Closed
@l3kn
Copy link
Owner

l3kn commented Sep 19, 2022

Looks good, I like how configurable this is now.

One caveat to keep in mind is that if Emacs is restarted, the daily limit is lost.
That's fine for me as long as we add this info to the docs,
persistence for the daily count / reset day can always be added in later.

Regarding testing it should be sufficient to mock different values for the daily count / reset day
and count the new cards from a selection of (unshuffled) cards.

@cashpw cashpw force-pushed the feat/daily-new-limit branch 2 times, most recently from 41e286d to 2249089 Compare September 19, 2022 15:42
@cashpw
Copy link
Contributor Author

cashpw commented Sep 19, 2022

One caveat to keep in mind is that if Emacs is restarted, the daily limit is lost. That's fine for me as long as we add this info to the docs, ...

Good point. I've updated the doc strings.

persistence for the daily count / reset day can always be added in later.

I've created #98 to track this work.

@cashpw cashpw mentioned this pull request Sep 19, 2022
Open
@cashpw
Copy link
Contributor Author

cashpw commented Oct 6, 2022

Checking in. Are there any changes you'd like to see before this is merged?

@cashpw cashpw changed the base branch from main to develop October 11, 2022 16:41
@cashpw
Copy link
Contributor Author

cashpw commented Oct 18, 2022

Checking in -- is there anything you'd like to see changed in this pull request?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@l3kn l3kn l3kn left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@cashpw @l3kn