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

add progress #174

Conversation

AlexErrant
Copy link
Member

Let's keep the discussion about this PR here.

@asukaminato0721
Copy link
Collaborator

 Caused by:
  failed to load source for dependency `burn`

Caused by:
  Unable to update /home/runner/work/fsrs-rs/burn/burn

Caused by:
  failed to read `/home/runner/work/fsrs-rs/burn/burn/Cargo.toml`

Caused by:
  No such file or directory (os error 2)

seems caused by sub-module or something else.

fsrs-rs/burn/burn/Cargo.toml

@L-M-Sherlock
Copy link
Member

It's relied on https://github.com/open-spaced-repetition/burn via submodule:

image

Copy link
Member

@L-M-Sherlock L-M-Sherlock left a comment

Choose a reason for hiding this comment

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

LGTM

@L-M-Sherlock L-M-Sherlock merged commit d74f7e9 into open-spaced-repetition:fsrs-browser Apr 3, 2024
3 checks passed
@AlexErrant
Copy link
Member Author

Hmm, a small noise of protest regarding the additional commits. Yes, they make the build in this repo pass. However that's not important since all that matters is the CI in fsrs-browser passing.

Worse, they greatly increase the chance of merge conflicts.

I've been trying to keep my changes to fsrs-rs small and minimal to make it super easy to merge main into fsrs-browser. With the most recent commits, any changes to any tests will cause a merge conflict that will constantly require manual work to merge. I was thinking about using a Github action like the one described here to automatically keep the fsrs-browser branch up to date with main. The conflicts in the tests will prevent this from happening. If we have a fsrs-browser branch that is automatically kept up to date with main, we could then have a cron-job that will automatically update the fsrs-browser repo with the latest fsrs-rs. That would be cool since then you wouldn't need to constantly make PRs to fsrs-browser. Any changes to fsrs-rs's main would automatically be propagated to fsrs-browser without any manual PRs. (Once I write some tests to prevent broken releases of course.)

If you really want to "delete" the tests, I recommend commenting them out with \* code *\ since it will make it unlikely to cause merge conflicts.

Actually, I think the preferred solution would be to add an exception for running CI for the fsrs-browser branch in this repo. Again, all that matters is the CI in the fsrs-browser repo.

Side note, prioritizing reduction of merge conflicts is also the reason why I haven't been fixing any warnings, though my editor is constantly screaming at me about all the unused code 😅

@L-M-Sherlock
Copy link
Member

I haven't thought about that, sorry. Could we revert this PR?

By the way, FSRS-rs will be stable recently, because I will focus on the integration of the fsrs4anki helper add-on into Anki. So the back merge will happen less frequently.

Thanks to your great contributions again.

@AlexErrant
Copy link
Member Author

It's all good! I didn't spell out any of my thoughts about this anywhere, so at least now we're on the same page!

@dae
Copy link
Collaborator

dae commented Apr 4, 2024

@L-M-Sherlock as a first step towards that, do you think you could write a quick outline of how you plan to implement them at one point? My main concern with integration into Anki is that we don't add a lot of processing time between card answers, so for example it would be good to build a cache of how many cards are due on each day, instead of scanning the database for due cards on each answer.

@L-M-Sherlock
Copy link
Member

@dae, let's talk about it in ankitects/anki#3116

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.

4 participants