Fix output for multiline column comments #779

tmr08c · 2020-03-22T01:58:10Z

Closes #778

If a column comment includes the newline character, the newline character
would be "printed" into the annotation block resulting in a line break
and an uncommented line.

For example, for the following table:

create_table "users", force: :cascade do |t|
  t.string "name", comment: "This is a comment.\nWith two lines!"
  t.datetime "created_at", precision: 6, null: false
  t.datetime "updated_at", precision: 6, null: false
end

annotating the model with the --with-comment flag will result in:

\# == Schema Information
\#
\# Table name: users
\#
\#  id                                       :bigint           not null, primary key
\#  name(This is a comment.
With two lines!) :string
\#  created_at                               :datetime         not null
\#  updated_at                               :datetime         not null
\#

This uncommented line would result in invalid Ruby and cause the file to
no longer be valid.

This fix replaces the newline character with an escaped version, so the
output will look more like:

\# == Schema Information
\#
\# Table name: users
\#
\#  id                                       :bigint           not null, primary key
\#  name(This is a comment.\nWith two lines!):string
\#  created_at                               :datetime         not null
\#  updated_at                               :datetime         not null
\#

lib/annotate/annotate_models.rb

tmr08c · 2020-03-22T02:08:17Z

lib/annotate/annotate_models.rb

        col_name = if with_comments?(klass, options) && col.comment
-                     "#{col.name}(#{col.comment})"
+                     "#{col.name}(#{col.comment.gsub(/\n/, "\\n")})"


I wanted to find something more general and elegant than gsub, but didn't find a good option. I'd be interested if there are other characters that would be concerning (\t?) that would make sense to check for and escape as well.

Maybe there's a library for sanitizing whitespace? It might be worth looking into.

I looked a bit, but didn't find anything that stuck out. String#dump seemed promising, but I believe it escaped UTF characters that were expected to work.

I could make this more generic and escape any \ with \\ if that seems safer.

drwl · 2020-04-05T07:40:11Z

Woah neat, I didn't know column comments were a thing.

drwl · 2020-04-05T07:42:59Z

@tmr08c could you rebase and add comments to make the rubocop step part of CI pass? Happy to merge in after.

This is a bit of a cheat of a refactoring that simply extracts the logic for collecting a column's attributes out of `get_schema_info` and into its own method (`get_attributes`). I found that in PRs like #779 that the Rubocop ABC limit was being exceeded: ``` lib/annotate/annotate_models.rb:235:5: C: Metrics/AbcSize: Assignment Branch Condition size for get_schema_info is too high. [145/145] def get_schema_info(klass, header, options = {}) ... ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ ``` Hopefully, this should break this up and reduce the complexity of the method. There are opportunities to go further, but I thought this could be a good place to start. I would be open and interested in discussing further refactoring opportunities if it would make sense (maybe creating some new classes to encapsulate some of this logic).

If a column comment includes the newline character, the newline character would be "printed" into the annotation block resulting in a line break and an uncommented line. For example, for the following table: ``` create_table "users", force: :cascade do |t| t.string "name", comment: "This is a comment.\nWith two lines!" t.datetime "created_at", precision: 6, null: false t.datetime "updated_at", precision: 6, null: false end ``` annotating the model with the `--with-comment` flag will result in: ``` \# == Schema Information \# \# Table name: users \# \# id :bigint not null, primary key \# name(This is a comment. With two lines!) :string \# created_at :datetime not null \# updated_at :datetime not null \# ``` This uncommented line would result in invalid Ruby and cause the file to no longer be valid. This fix replaces the newline character with an escaped version, so the output will look more like: ``` \# == Schema Information \# \# Table name: users \# \# id :bigint not null, primary key \# name(This is a comment.\nWith two lines!):string \# created_at :datetime not null \# updated_at :datetime not null \# ```

This is a bit of a cheat of a refactoring that simply extracts the logic for collecting a column's attributes out of `get_schema_info` and into its own method (`get_attributes`). I found that in PRs like ctran#779 that the Rubocop ABC limit was being exceeded: ``` lib/annotate/annotate_models.rb:235:5: C: Metrics/AbcSize: Assignment Branch Condition size for get_schema_info is too high. [145/145] def get_schema_info(klass, header, options = {}) ... ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ ``` Hopefully, this should break this up and reduce the complexity of the method. There are opportunities to go further, but I thought this could be a good place to start. I would be open and interested in discussing further refactoring opportunities if it would make sense (maybe creating some new classes to encapsulate some of this logic).

Closes ctran#778 If a column comment includes the newline character, the newline character would be "printed" into the annotation block resulting in a line break and an uncommented line. For example, for the following table: ``` create_table "users", force: :cascade do |t| t.string "name", comment: "This is a comment.\nWith two lines!" t.datetime "created_at", precision: 6, null: false t.datetime "updated_at", precision: 6, null: false end ``` annotating the model with the `--with-comment` flag will result in: ``` \# == Schema Information \# \# Table name: users \# \# id :bigint not null, primary key \# name(This is a comment. With two lines!) :string \# created_at :datetime not null \# updated_at :datetime not null \# ``` This uncommented line would result in invalid Ruby and cause the file to no longer be valid. This fix replaces the newline character with an escaped version, so the output will look more like: ``` \# == Schema Information \# \# Table name: users \# \# id :bigint not null, primary key \# name(This is a comment.\nWith two lines!):string \# created_at :datetime not null \# updated_at :datetime not null \# ```

This is a bit of a cheat of a refactoring that simply extracts the logic for collecting a column's attributes out of `get_schema_info` and into its own method (`get_attributes`). I found that in PRs like ctran#779 that the Rubocop ABC limit was being exceeded: ``` lib/annotate/annotate_models.rb:235:5: C: Metrics/AbcSize: Assignment Branch Condition size for get_schema_info is too high. [145/145] def get_schema_info(klass, header, options = {}) ... ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ ``` Hopefully, this should break this up and reduce the complexity of the method. There are opportunities to go further, but I thought this could be a good place to start. I would be open and interested in discussing further refactoring opportunities if it would make sense (maybe creating some new classes to encapsulate some of this logic).

Closes ctran#778 If a column comment includes the newline character, the newline character would be "printed" into the annotation block resulting in a line break and an uncommented line. For example, for the following table: ``` create_table "users", force: :cascade do |t| t.string "name", comment: "This is a comment.\nWith two lines!" t.datetime "created_at", precision: 6, null: false t.datetime "updated_at", precision: 6, null: false end ``` annotating the model with the `--with-comment` flag will result in: ``` \# == Schema Information \# \# Table name: users \# \# id :bigint not null, primary key \# name(This is a comment. With two lines!) :string \# created_at :datetime not null \# updated_at :datetime not null \# ``` This uncommented line would result in invalid Ruby and cause the file to no longer be valid. This fix replaces the newline character with an escaped version, so the output will look more like: ``` \# == Schema Information \# \# Table name: users \# \# id :bigint not null, primary key \# name(This is a comment.\nWith two lines!):string \# created_at :datetime not null \# updated_at :datetime not null \# ```

tmr08c commented Mar 22, 2020

View reviewed changes

lib/annotate/annotate_models.rb Show resolved Hide resolved

tmr08c commented Mar 22, 2020

View reviewed changes

tmr08c mentioned this pull request Apr 4, 2020

Reactors AnnotateModels.get_schema_info #791

Merged

tmr08c force-pushed the tmr08c-fix-multi-line-comments branch from 74a2075 to 2e6c777 Compare April 5, 2020 14:55

drwl approved these changes Apr 5, 2020

View reviewed changes

drwl merged commit 214da4f into ctran:develop Apr 5, 2020

tmr08c deleted the tmr08c-fix-multi-line-comments branch April 6, 2020 10:40

This was referenced Jun 1, 2021

Multi-line column comments break annotated comment block #866

Open

When is the next release scheduled? #881

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix output for multiline column comments #779

Fix output for multiline column comments #779

tmr08c commented Mar 22, 2020

tmr08c Mar 22, 2020

drwl Apr 5, 2020

tmr08c Apr 10, 2020

drwl commented Apr 5, 2020

drwl commented Apr 5, 2020

Fix output for multiline column comments #779

Fix output for multiline column comments #779

Conversation

tmr08c commented Mar 22, 2020

tmr08c Mar 22, 2020

Choose a reason for hiding this comment

drwl Apr 5, 2020

Choose a reason for hiding this comment

tmr08c Apr 10, 2020

Choose a reason for hiding this comment

drwl commented Apr 5, 2020

drwl commented Apr 5, 2020