-
Notifications
You must be signed in to change notification settings - Fork 0
Optimize key formatter #6
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
Changes from 5 commits
7c58bcc
b524e6c
47824af
a6f1eed
964ef0b
34814e7
5ea899b
be609e7
e0d60fd
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,32 +1,28 @@ | ||
| require 'jbuilder/jbuilder' | ||
| require 'active_support/core_ext/array' | ||
|
|
||
| class Jbuilder | ||
| class KeyFormatter | ||
| def initialize(*args) | ||
| @format = {} | ||
| @cache = {} | ||
|
|
||
| options = args.extract_options! | ||
| args.each do |name| | ||
| @format[name] = [] | ||
| end | ||
| options.each do |name, parameters| | ||
| @format[name] = parameters | ||
| end | ||
| end | ||
|
|
||
| def initialize_copy(original) | ||
| def initialize(*formats, **formats_with_options) | ||
| @mutex = Mutex.new | ||
| @formats = formats | ||
| @formats_with_options = formats_with_options | ||
| @cache = {} | ||
| end | ||
|
|
||
| def format(key) | ||
| @cache[key] ||= @format.inject(key.to_s) do |result, args| | ||
| func, args = args | ||
| if ::Proc === func | ||
| func.call result, *args | ||
| else | ||
| result.send func, *args | ||
| @mutex.synchronize do | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It's very annoying that ruby doesn't come native with a RW lock (esp an upgradable one) cause this would be a perfect case for one given the expected use patterns. Might be some check-lock-check patterns that might be a bit faster under high contention but not worth exploring atm. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It doesn't, but the concurrent gem provides one, and we use it already in the app: https://ruby-concurrency.github.io/concurrent-ruby/1.1.5/Concurrent/ReadWriteLock.html There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We can't fork and then add deps I don't think and achieve the goal of compat There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. yeah I know |
||
| @cache[key] ||= begin | ||
| value = key.is_a?(Symbol) ? key.name : key.to_s | ||
|
|
||
| @formats.each do |func| | ||
| value = func.is_a?(Proc) ? func.call(value) : value.send(func) | ||
| end | ||
|
|
||
| @formats_with_options.each do |func, params| | ||
| value = func.is_a?(Proc) ? func.call(value, *params) : value.send(func, *params) | ||
| end | ||
|
|
||
| value | ||
| end | ||
| end | ||
| end | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -784,14 +784,6 @@ class JbuilderTest < ActiveSupport::TestCase | |
| assert_equal ['camelStyle'], result.keys | ||
| end | ||
|
|
||
| test 'do not use default key formatter directly' do | ||
| Jbuilder.key_format | ||
| jbuild{ |json| json.key 'value' } | ||
| formatter = Jbuilder.send(:class_variable_get, '@@key_formatter') | ||
| cache = formatter.instance_variable_get('@cache') | ||
| assert_empty cache | ||
| end | ||
|
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Maybe repurpose this to validate that it is used?
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Suppose I should just repurpose this test. With the |
||
|
|
||
| test 'ignore_nil! without a parameter' do | ||
| result = jbuild do |json| | ||
| json.ignore_nil! | ||
|
|
||
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.
My ruby is bad now - will this be nil if it's unset which I know is falsy but..
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.
optionscan't be unset. It is a required parameter that defaults tonil.I can change this to
to make it clearer (and perhaps safer).
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.
Actually... I wonder if it's better to just use
kwargshere... hold on, I think I got something better.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.
I mean the value in
:ignore_nilis a nil if it's not there right?Uh oh!
There was an error while loading. Please reload this page.
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.
Yeah,
Hashes will returnnilif the key doesn't exist. Should have beenfalse. Likewise with: deep_format_keys. I was too careless hammering this out.Moot point. Moved it over to use key args, which actually speeds things up a bit more.