Skip to content
This repository has been archived by the owner on Jul 13, 2023. It is now read-only.

Fixed only_process option with proc #2289

Merged
merged 1 commit into from
Aug 28, 2016
Merged

Fixed only_process option with proc #2289

merged 1 commit into from
Aug 28, 2016

Conversation

morgoth
Copy link
Contributor

@morgoth morgoth commented Aug 24, 2016

Bug introduced in 1c7d7f6

Per 1c7d7f6#commitcomment-18765681 request /cc @tute

@@ -240,7 +240,8 @@ def dirty?
# the instance's errors and returns false, cancelling the save.
def save
flush_deletes unless @options[:keep_old_files]
if @options[:only_process].any? && !@options[:only_process].include?(:original)
process = only_process
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Maybe it would be better to memoize only_process method?
If it's called somewhere else, proc will be computed again. What do you think?

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe it would be better to memoize only_process method?
If it's called somewhere else, proc will be computed again. What do you think?

Could that present regressions? For now, I'd rather not cache it, and wait for it to actually become a bottleneck to discuss if we should go down that road. Thanks!

@tute tute merged commit d5eca24 into thoughtbot:master Aug 28, 2016
@tute
Copy link
Contributor

tute commented Aug 28, 2016

Thanks so much! 🎉

tute pushed a commit that referenced this pull request Aug 28, 2016
@jaybloke
Copy link

jaybloke commented Oct 2, 2016

Regardless of the contents of only_process, shouldn't the original file always be written to the file system?

I recently raised an issue when using the latest version of Paperclip with Delayed Paperclip - jrgifford/delayed_paperclip#199

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants