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

Customizable key-value separator and quoting to be machine-readable #130

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

modulitos
Copy link

@modulitos modulitos commented Feb 6, 2022

This pull request surfaces a new optional configuration to format the Marginalia comments in a machine-readable manner. See the changes to the README in this PR for a summary of the new feature.

Note that this PR is backwards-compatible with existing code. The new setting is optional, and if unset, the default values will retain all existing behavior. Unit tests have also be added to ensure there are no regressions.

Further notes:

  • The sqlcommenter project references this fork of Marginalia
  • Here is a previous pull request that attempted to add this same feature to Marginalia: https://github.com/basecamp/marginalia/pull/89/commits
    • That pull request had favorable reviews, but seemed to fizzle out due to merge conflicts.
    • The only difference is that it resolves some merge conflicts and adds some testing.
  • My company has been running this fork of Marginalia in production since March 3rd, and we haven't had any issues.
  • I'm more than happy to give anyone a walkthrough of the changes, or buy a coffee, or make a donation for your time :)

TODO:

Note that some changes were made to the API of this feature over the course of this PR. Here is the original description, for posterity:

This pull request provides 2 new optional configs which are backwards compatible with all existing code by default.
The 2 optional configs will adjust the formatting of Marginalia's SQL comments:

  1. key_value_separator: inject the key-value separator, which is a colon : by default
  2. quote_values: configure whether the values will be wrapped in a single quoted string. By default, this is false, which will not wrap the values at all.

For example, if a client uses the following configuration:

Marginalia::Comment.key_value_separator = '='
Marginalia::Comment.quote_values = :single

it will adjust the formatting from this:

"select id from posts /*application:Joe's app,controller:my_controller*/"

to this:

"select id from posts /*application='Joe\\'s app',controller='my_controller*/"

Note that this PR is backwards-compatible with existing code. If neither quote_values nor key_value_separator are set, their default values will retain all existing behavior. Unit tests have also be added to ensure there are no regressions.

@modulitos
Copy link
Author

modulitos commented Feb 7, 2022

@sj26 I would love to get a review on this PR when you have a chance. Or let me know if there's anything I can do to help get this reviewed.

@modulitos
Copy link
Author

Kindly pinging the marginalia authors (@arthurnn @jeremy) - is there anything I can do to help get this PR reviewed?

Also pinging @balachandr and @odeke-em, as you both had comments in the original PR: #89

I did my best to write up a concise description of the new behavior, and demonstrate that it's all backwards compatible.

@modulitos modulitos changed the title Customizable key-value separator and quoting Customizable key-value separator and quoting to be machine-readable Mar 10, 2022
Copy link
Member

@jeremy jeremy left a comment

Choose a reason for hiding this comment

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

Would approach this as configuring a formatter rather than as configuring the low-level details of key/value output.

ensure
prev.each { |(key, value)| Marginalia::Comment.send(:"#{key}=", value) }
end

Copy link
Member

Choose a reason for hiding this comment

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

Appears to be a test helper. Would move this to a utility method on the test case rather than introduce new toplevel API.

Copy link
Author

Choose a reason for hiding this comment

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

Thanks for catching this 💯

I ended up removing this test helper, since it introduces indirection that doesn't seem worthwhile.

def self.construct_comment
ret = String.new
self.components.each do |c|
component_value = self.send(c)
if component_value.present?
ret << "#{c}:#{component_value},"
ret << "#{c}#{key_value_separator}"\
"#{quote_value(component_value)},"
Copy link
Member

Choose a reason for hiding this comment

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

Suggest introducing a configurable formatter block rather than separate low-level config points, including :default and :sqlcommenter shorthands for "standard" formatters.

Copy link
Author

Choose a reason for hiding this comment

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

Done 👍

I added support for configuring "standard" formatters like :sqlcommenter directly via Marginalia::Comment.update_formatter!(:sqlcommenter), and I removed configuration for key_value_separator and quote_value directly on the Comment module since it seemed like YAGNI.

I agree that those low-level config points should be managed together, so I introduced a FormatterFactory that handles the composition of these configs instead. I think this results in a better separation of concerns between the formatting and commenting logic as well. Open to feedback, of course :)

Copy link
Author

@modulitos modulitos left a comment

Choose a reason for hiding this comment

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

Thanks @jeremy - I appreciate the great feedback!

Your suggestions led me to think deeper on the formatting logic, resulting in improvements to the API and code organization. Looking forward to your thoughts on the latest changes.

ensure
prev.each { |(key, value)| Marginalia::Comment.send(:"#{key}=", value) }
end

Copy link
Author

Choose a reason for hiding this comment

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

Thanks for catching this 💯

I ended up removing this test helper, since it introduces indirection that doesn't seem worthwhile.

def self.construct_comment
ret = String.new
self.components.each do |c|
component_value = self.send(c)
if component_value.present?
ret << "#{c}:#{component_value},"
ret << "#{c}#{key_value_separator}"\
"#{quote_value(component_value)},"
Copy link
Author

Choose a reason for hiding this comment

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

Done 👍

I added support for configuring "standard" formatters like :sqlcommenter directly via Marginalia::Comment.update_formatter!(:sqlcommenter), and I removed configuration for key_value_separator and quote_value directly on the Comment module since it seemed like YAGNI.

I agree that those low-level config points should be managed together, so I introduced a FormatterFactory that handles the composition of these configs instead. I think this results in a better separation of concerns between the formatting and commenting logic as well. Open to feedback, of course :)

end

desc "test sqlite3 driver"
task :sqlite do
sh "DRIVER=sqlite3 bundle exec ruby -Ilib -Itest test/*_test.rb"
sh "DRIVER=sqlite3 bundle exec ruby -Ilib -Itest test/query_comments_test.rb"
Copy link
Author

Choose a reason for hiding this comment

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

I changed these globs from test/*_test.rb to test/query_comments_test.rb since that's the only file that needs to be matched for the database tests. It also allows us to run the formatting tests separately.

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.

2 participants