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

refactor: Separate business logic from user interface code #751

Merged
merged 68 commits into from
Oct 4, 2023

Conversation

ronzulu
Copy link
Collaborator

@ronzulu ronzulu commented Sep 30, 2023

Based on https://github.com/st3v3nmw/obsidian-spaced-repetition/releases/tag/1.10.1

  • Separated out "business logic" from the user interface code for flashcards, as well as other refactoring.
  • The intention has been to keep existing functionality without change, but structure the code to support future contributions.
  • As part of the restructure, many unit test cases were added - now around 170 (up from 20)

Bug Fixes

To simplify the process of reviewing for the project owner, apart from keeping functionality the same, no bugs have been fixed. However, during the restructure and testing, it was noticed that the following was fixed:

  • [Fixed] Revisons cannot be saved properly #745
  • [Fixed] Decrease in Card Count after Pressing 'Reset' #704

Details

  • Development: Thoughts about implementation redesign? #736

Commit Comments

Much work went into this restructure. Until the home stretch, the commit comments weren't used (just "something")
There wouldn't be much to gain by examining any of the interim commits.

@ronzulu
Copy link
Collaborator Author

ronzulu commented Oct 1, 2023

Hi Stephen

I just noticed that the restructure has also fixed:

Cheers
Ronny

@st3v3nmw st3v3nmw changed the title Separated out "business logic" from the user interface code for flashcards, added 150 unit test cases refactor: Separate business logic from user interface code Oct 3, 2023
st3v3nmw
st3v3nmw previously approved these changes Oct 3, 2023
Copy link
Owner

@st3v3nmw st3v3nmw left a comment

Choose a reason for hiding this comment

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

Hi @ronzulu, I think this is good to go in. Please fix the linting issues.
Again, thanks for the awesome work! 🚀

@ronzulu
Copy link
Collaborator Author

ronzulu commented Oct 4, 2023

Hi Stephen, I've fixed all issues reported by pnpm lint. Cheers.

@ronzulu
Copy link
Collaborator Author

ronzulu commented Oct 4, 2023

Oops, forgot to mention that https://github.com/ronzulu/obsidian-spaced-repetition also now includes fixes for:

#706
#709

Cheers
Ronny

@st3v3nmw
Copy link
Owner

st3v3nmw commented Oct 4, 2023

Hi @ronzulu, please reduce the coverage thresholds for now so that this can go in

@ronzulu
Copy link
Collaborator Author

ronzulu commented Oct 4, 2023

Hi @st3v3nmw, just doing this now :-)

@st3v3nmw st3v3nmw merged commit 5e8f988 into st3v3nmw:master Oct 4, 2023
1 check passed
@st3v3nmw
Copy link
Owner

st3v3nmw commented Oct 4, 2023

🚀

@st3v3nmw
Copy link
Owner

st3v3nmw commented Oct 6, 2023

Hi @ronzulu, I'll do a release over the weekend

@ronzulu
Copy link
Collaborator Author

ronzulu commented Oct 6, 2023

Hi @st3v3nmw great 👍

I've been looking at https://github.com/st3v3nmw/obsidian-spaced-repetition/issues?q=is%3Aissue+is%3Aopen, and there are many open!

Let me know if there are any specific bugs/features I can help with! I'm currently working on #754.

Also, I think there are a fair few issues that are open that could be closed. If you like, I could mark any that I come across for your attention.

Cheers
Ronny

@st3v3nmw
Copy link
Owner

st3v3nmw commented Oct 9, 2023

Hi @ronzulu,

I did the release a few hours ago.

Yeah, there are a lot of open issues (life & work got in the way 🥲) but I'm hoping to start closing them slowly. I think your refactor will help speedup the process.

I'll create a new GitHub board to start tracking the bugs/features that are currently being worked on but I currently don't have any in mind.

Yes, please let me know if you find any open issues that could be closed.

Thank you!

@AB1908
Copy link
Contributor

AB1908 commented Oct 21, 2023

Jesus Christ, how long did it take to pull this off? You're a much braver person than I. I actually ended up rewriting most of the plugin and deprecating a bunch of features because I don't have the skills to do something like this. Given that you're familiar with the domain and the underlying logic, I would love to get some thoughts on my fork of this plugin.

Newdea pushed a commit to open-spaced-repetition/obsidian-spaced-repetition-recall that referenced this pull request Dec 1, 2023
)

* First commit

* This doesn't build yet

* Something

* Something

* Something

* Something

* Something

* Something

* Something

* Something

* Something

* Something

* Something

* Something

* Something

* Something

* Something

* Something

* Something

* Something

* Something

* Something

* Something

* Something

* Something

* Something

* Something

* Something

* Something

* Something

* Something

* Something

* Something

* Something

* Something

* Something

* Something

* Something

* Something

* Something

* Something

* Something

* Something

* Something

* Added support for burySiblingCards

* Fixed the card count displayed on the status bar

* Minor

* Minor

* Refactoring in preparation for random DeckTreeIter

* Support random sequencing of cards during review

* Fixed statistics charts

* after npm run format

* Code for updating question text made resilient to whitespace differences

* Added more test cases

* Added test case

* Fixes for certain cases of whitespace following a question's topic tag

* npm run format; update version in manifest.json

* Support the Enter key on the numeric keypad in the shortcuts st3v3nmw#706

* In flashcard modal, allow user to click across entire tree item rectangle st3v3nmw#709

* npm run format; temporary filename changes

* Updated formatting for lint, file rename

* fix issues reported by pnpm lint

* Restored version number to "1.10.1"

* Restored pnpm-lock.yaml from version 1.10.1

* Updated change log to reference st3v3nmw#706, and st3v3nmw#709

* Fixing capitalization of "Note.ts" (step 1)

* Fixing capitalization of "Note.ts" step #2

* Reduced collectCoverageFrom to subset of files with 100% code coverage
@ronzulu
Copy link
Collaborator Author

ronzulu commented Jun 15, 2024

Hi @AB1908 I didn't record the time spent, but definitely a lot of time! I'm guessing at least the equivalent of 6 full-time weeks spread over maybe 12. The main issue was that I only used a portion of the plug-in functionality myself, and so there were a fair few bugs that I introduced, which usually weren't difficult to fix. But I always added unit test cases, so hopefully long-term the product quality is higher.

I'm now working on Refactor code to support diff methods of storing the scheduling info, and diff SR algorithms #878, This has also required significant work, hopefully at the tail end now.

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.

3 participants