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

Extract caching into its own module. #1402

Merged
merged 1 commit into from
Dec 28, 2015
Merged

Conversation

beauby
Copy link
Contributor

@beauby beauby commented Dec 27, 2015

No description provided.

module ClassMethods
def inherited(base)
super
caller_line = caller.second
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Note that now that caching is inside a mixin, the actual serializer definition is in the second position in the call stack.

Copy link
Member

Choose a reason for hiding this comment

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

Could still be the below, right?

def inherited(base)
  caller_line = caller.first
  super
  base._cache_digest = digest_caller_file(caller_line)
end

Also, I'd rather use caller[1] than second just because I prefer to avoid core extensions. I'm not sure if you have a preference.

Copy link
Member

Choose a reason for hiding this comment

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

though, we should probably rename the local var to serializer_source_location

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nope, the Caching#inherited method is always called from Serializer#inherited. I did make the change to caller[1] though.

@bf4
Copy link
Member

bf4 commented Dec 28, 2015

💯 I'm not big on the extra-a-module pattern, but I really think this helps us better understand the parts of the serializer public api. Interestingly, there was once a Serializer::DSL object, but it was only in the refactor branch.

@bf4
Copy link
Member

bf4 commented Dec 28, 2015

I'm ok to merge this once tests pass

@beauby
Copy link
Contributor Author

beauby commented Dec 28, 2015

Tests passed, I'm merging as @bf4 agreed.

beauby added a commit that referenced this pull request Dec 28, 2015
Extract caching into its own module
@beauby beauby merged commit 12cd190 into rails-api:master Dec 28, 2015
@bf4 bf4 deleted the extract-caching branch December 28, 2015 03:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants