-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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
regard :from_version option on calling #recreate_versions! with args #1879
regard :from_version option on calling #recreate_versions! with args #1879
Conversation
@@ -518,6 +518,26 @@ User.find_each do |user| | |||
end | |||
``` | |||
|
|||
### Recreating versions with `:from_version` dependency |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please move this to the wiki, the Readme is already too big for a very specific user case
@hedgesky I've added a couple of comments, I would appreciate very much if you go forward with it, there's definitely some value laying there. 👍 |
|
||
def set_versions_to_cache_and_store(names) | ||
@versions_to_cache = source_versions_of(names) | ||
@versions_to_store = active_versions_with_names_in(@versions_to_cache + names) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Basically, it's still an instance variable. And now even two of them instead of one 😄 . But introducing them helps to avoid conditions in cache_versions!
and store_versions!
methods. Also, assigning these variables is hidden in methods with readable names. So, I think this change decreases overall complexity.
@thiagofm Thanks for looking into that! Following your suggestions, I've made some changes. As for me, it's much readable now. |
else | ||
cache! if !cached? | ||
store! | ||
end | ||
end | ||
|
||
private | ||
|
||
def set_versions_to_cache_and_store(names) | ||
@versions_to_cache = source_versions_of(names) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Those instance variables being set here makes me think that the recreate_versions could be in its own class. As of now, I guess it's good to avoid adding more code here. Do you mind giving it a try @hedgesky ?
@thiagofm Sorry for late answer. I'll try it when I have spare time. I agree with you that this functionality should be extracted but don't see right now how it should be implemented properly. Will think about it. |
Merging in finally, thanks! |
@mshibuya When are we going to have this released? |
Fixes #1164. Maybe it's also related to #1602.