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

Move code into a Bundler style gem package #2

Merged
merged 4 commits into from Apr 10, 2017
Merged

Move code into a Bundler style gem package #2

merged 4 commits into from Apr 10, 2017

Conversation

scottharvey
Copy link
Contributor

This PR moves all the existing Repetition code into a more standard gem structure.

I did this by running bundler gem repetition and then moving the code into the generated code. This has the added advantage of making specs easier to run by calling bundler exec rake

I've also updated the README to include an example Rails migration and information on how to run the specs.

Finally I've updated an error message in lib/repetition.rb which incorrectly stated you could pass a value of 0 into .process_recall_result

@dankimio dankimio merged commit 52b4ccf into dankimio:master Apr 10, 2017
@dankimio
Copy link
Owner

Thank you for the pull request!

@scottharvey
Copy link
Contributor Author

@dankimio Are you using this gem anywhere? I've got a couple ideas how to change it but I'm not sure if I should keep creating PRs or just maintain my own fork.

@dankimio
Copy link
Owner

dankimio commented Apr 10, 2017

I'm open to new ideas! What are the changes that you want to make?

@scottharvey
Copy link
Contributor Author

scottharvey commented Apr 10, 2017

I would like to remove reset_spaced_repetition_data and instead initialize the attributes inside process_recall_result when needed.

Also, I want to be able to pass a date into process_recall_result so I can specify a date other than Date.today

Finally, I found a bug which meant repetition_interval wasn't being initialized properly in a certain case which was causing an error.

I can create a PR for each one if you are interested.

@dankimio
Copy link
Owner

Sounds like it's more reasonable to move the whole module into a separate class. What do you think? Using the gem as a mixin adds a lot of constraints and probably was not a good idea.

So it would look like the following:

# Initialize
options = { easiness_factor: 2.5, schedule_from: Date.today, ... }
flashcard = Repetition::Flashcard.new(options)

# Result (recall result is calculated under the hood)
flashcard.next_repetition # => Date

Thus, it would be possible to initialize attributes in class initializer and we can include other options, such as date override as well.

PRs for bugs are always welcome!

@scottharvey
Copy link
Contributor Author

Yeah, it might be nice to have it as a stand alone class so it's nice a clean. But then you could could also have some extra railties code so it fits in nicely with Rails / Active Record.

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.

None yet

2 participants