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

Add support for json comment #10

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

Conversation

jfrancoist
Copy link

@jfrancoist jfrancoist commented Apr 2, 2020

This PR adds support for JSON comment format. The current behaviour stays as default to avoid any breaking change.

To enable JSON support the gem needs to be configured during application initialisation.

ActiveRecord::Comments.configure do |config|
  config.enable_json_comment = true
end

Having JSON comments allows for easier parsing with readily available libs instead of using regex to extract comments.

@jfrancoist jfrancoist force-pushed the jtopige_add-support-for-json-comment-2 branch from 9d1c1b1 to d1c66dd Compare April 2, 2020 12:48
lib/json_commenter.rb Outdated Show resolved Hide resolved
lib/json_commenter.rb Outdated Show resolved Hide resolved
lib/json_commenter.rb Outdated Show resolved Hide resolved
@jfrancoist jfrancoist force-pushed the jtopige_add-support-for-json-comment-2 branch 2 times, most recently from a99c9b8 to 93a8f7b Compare April 3, 2020 09:58
@jfrancoist jfrancoist marked this pull request as ready for review April 3, 2020 11:03
The JsonCommenter will generate JSON comments instead of a simple comma
seperated string.
@jfrancoist jfrancoist force-pushed the jtopige_add-support-for-json-comment-2 branch from 93a8f7b to ef9d118 Compare April 3, 2020 11:17
@grosser
Copy link
Owner

grosser commented Apr 3, 2020

hmm lots of complexity ... how about the commenter collects all the info and then merges all the hashes and then serializes them to json ... strings etc stay the same

@bquorning
Copy link
Contributor

bquorning commented Apr 6, 2020

If you had just one commenter class, you could call it with a mix of strings and hashes – which would become a mess. Isn’t it better to have the logic separated, as suggested by the current PR?

@grosser
Copy link
Owner

grosser commented Apr 6, 2020 via email

@jfrancoist
Copy link
Author

In general having one commenter that would merge both strings and hashes together might not be problem, if on the other end a "human" is reading the comments.

On the other end if some application needs to make use of the comments for some task/analysis, it would make it much more difficult to interpret scenarios such as:

SELECT * FROM Users /* foo:bar, hello:world {"foo":zoo","hello":goodbye"} */

By having a clear separation of concerns we make sure existing use-case is not broken and enabling JSON comment is configurable.

In my opinion there should be only one transition path which is to either switch from base commenter to JSON or vis-versa, having a mix of both might lead to confusion or a bad practice of having two different comment format within the same comment block.

The goal of this PR is mainly to provide a "safe" way for tools to parse comments without the need of complex REGEX.

@grosser
Copy link
Owner

grosser commented Apr 8, 2020

if we are going down the "it needs to be either or" route:

  • don't need a configurator, set with a cattr_accessor ?
  • keep the collection the same (just array management) and then make the serialization different based on the flag / don't add multiple classes and instruction blocks

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.

3 participants