Fixes updating columns when using alternative formats #784

tmr08c · 2020-03-24T17:34:45Z

This resolves #780

Updates column_pattern in AnnotateModels.annotate_one_file to
support RDoc and Yard doc formats. column_pattern is responsible for
searching for existing annotations in the file - if the pattern is not
matched in the file, annotations are not updated.

Since the previous regular expression did not match the Yard
(@!) or RDoc (**<attributed>**::), when attempting to update
annotations for a model, it would look like there were no changes since
parsing the annotation blocks with the regular expression would return
nothing.

Runs annotate on 'rake db:rollback' too

Revert "Runs annotate on 'rake db:rollback' too"

tmr08c · 2020-04-11T01:53:08Z

lib/annotate/annotate_models.rb

@@ -362,7 +366,7 @@ def annotate_one_file(file_name, info_block, position, options = {})
      old_header = old_content.match(header_pattern).to_s
      new_header = info_block.match(header_pattern).to_s

-      column_pattern = /^#[\t ]+[\w\*\.`]+[\t ]+.+$/
+      column_pattern = /^#[\t ]+[\w\*\.\`\@\!\:]+[\t ]+.+$/


I was originally going to break this regular expression up into something like:

def column_pattern_for(format_type) case format_type when :yard then // when :rdoc then // # etc... end end

By the time I go to the end, I saw that the original regular expression could be updated and cover all cases.

I could see an advantage to breaking this up, but didn't want to go too far with refactoring and delay sharing this fix.

@tmr08c if you're still interested in getting this patch in, I think there's value into breaking it up. The column pattern regex is hard to understand already.

Thanks for the review and feedback @drwl! Working on updating this PR and adding some additional tests.

Updates `column_pattern` in `AnnotateModels.annotate_one_file` to support RDoc and Yard doc formats. `column_pattern` is responsible for searching for existing annotations in the file - if the patten is not matched in file, annotations are not updated. Since the previous regular expression did not match the Yard (`@!`) or RDoc (`**<attributed>**::`), when attempting to update annotations for a model, it would look like there were no changes since parsing the annotation blocks with the regular expression would return nothing.

tmr08c · 2020-04-11T02:26:34Z

@tvallois since you added Yard support (#724) and @Samsinite since you were interested in YARD support (#720), I figured I'd ping you both to see if you had any thoughts around this change or experienced this issue.

drwl · 2020-05-08T02:59:34Z

lib/annotate/annotate_models.rb

@@ -362,7 +366,7 @@ def annotate_one_file(file_name, info_block, position, options = {})
      old_header = old_content.match(header_pattern).to_s
      new_header = info_block.match(header_pattern).to_s

-      column_pattern = /^#[\t ]+[\w\*\.`]+[\t ]+.+$/
+      column_pattern = /^#[\t ]+[\w\*\.\`\@\!\:]+[\t ]+.+$/


@tmr08c if you're still interested in getting this patch in, I think there's value into breaking it up. The column pattern regex is hard to understand already.

…ng-yard-doc

…-doc

* Creates a method that checks the options to determine the current format option being used and return a symbol representation. * Breaks up the single regular expression used for determining if a line was an annotation for a column definition into a method that will `case` on the different format option.

* Adds tests for updating the type of a column * Fixes the regular expressions for `rdoc` and `yard` annotation types

tmr08c · 2020-05-09T02:02:20Z

lib/annotate/annotate_models.rb

+    #   annotations
+    def column_pattern_for(format_type)
+      case format_type
+      when :markdown then /^#\s+\*{2}`\w+`\*{2}(?:\s+\|\s`.*`)*$/


I went for stricter matches. I'm not sure if it would make sense to be a bit looser and do something like:

/^#\s+\*{2}`\w+.*$/

that would catch

# **column_name** ...

Currently, I'm checking for the fact there may be additional columns in the markdown table

After revisiting this PR, I'm not sure how I feel about this. I'm sure if we just grabbing all lines in the annotation section would end up being a problem.

The previous implementation seemed to sort of be checking that the comment had an indication that it was the "column section" (e.g., # col_name or # * col_name). This PR goes a bit further and does some stricter checking based on the format.

If we pulled in any non-empty comment line, it seems it shouldn't cause a problem since we are essentially checking for any difference. If we pulled in non-column comments, we'd get them in both old_columns and new_columns, so it shouldn't be flagged as a change.

I think the situation that we are avoiding by doing it this way is if there are non-annotation comments in the annotation block since it looks like the header_pattern grabs all groups comments after the Table name: comment.

tmr08c · 2020-05-09T02:03:02Z

lib/annotate/annotate_models.rb

+      when :markdown then /^#\s+\*{2}`\w+`\*{2}(?:\s+\|\s`.*`)*$/
+      when :rdoc then
+        field_name_regex = /^#\s+\*\w+\*\:{2}/
+        field_info_regex = /\s+\<tt\>.*\<\/tt\>$/


Similar to above, rather than checking the <tt>...</tt> this could all probably be .*$

tmr08c · 2020-05-09T02:04:25Z

lib/annotate/annotate_models.rb

+
+        /#{attribute_regex}|#{return_regex}/
+      else # :bare/default
+        /^#[\t ]+[\w\*\.\`\@\!\:]+[\t ]+.+$/


This is the catch-all regular expression. I would like to move it to something more specific to the bare format.

My initial implementation broke something for foreign keys, so I will need to see how that is handled for the various formats.

Updates the regular expression to more explicitly match the bare case and no longer be the general purpose regex.

- Adds markdown support (and test) - Fixes typo in `#annotate_one_file`

- Appends `_for` to match other regex method - Fixes check in non-bareword case to not capture the `.`. This would reduce the captures being returned instead of whether the line was matched overall

…g-yard-doc

tmr08c

The PR ended up being a bit bigger than I expected. Please let me know if it'd be useful to add PR comments to explain any tricky sections.

I'm interested in feedback on whether the new regular expressions are too rigid.

Initially, I liked the idea of taking advantage of generating different regular expressions for each format and creating more specific regular expressions to more closely check for the column syntax. Now, I'm less sure and worry it may end up being brittle.

tmr08c · 2020-05-17T22:03:00Z

lib/annotate/annotate_models.rb

+    #   annotations
+    def column_pattern_for(format_type)
+      case format_type
+      when :markdown then /^#\s+\*{2}`\w+`\*{2}(?:\s+\|\s`.*`)*$/


After revisiting this PR, I'm not sure how I feel about this. I'm sure if we just grabbing all lines in the annotation section would end up being a problem.

The previous implementation seemed to sort of be checking that the comment had an indication that it was the "column section" (e.g., # col_name or # * col_name). This PR goes a bit further and does some stricter checking based on the format.

If we pulled in any non-empty comment line, it seems it shouldn't cause a problem since we are essentially checking for any difference. If we pulled in non-column comments, we'd get them in both old_columns and new_columns, so it shouldn't be flagged as a change.

I think the situation that we are avoiding by doing it this way is if there are non-annotation comments in the annotation block since it looks like the header_pattern grabs all groups comments after the Table name: comment.

tmr08c · 2020-05-17T22:06:27Z

spec/lib/annotate/annotate_models_spec.rb

+          expect(File.read(@model_file_name)).to eq("#{@schema_info}#{@file_content}")
+        end
+
+        context 'when option "format_bare" is true' do


I started going down the shared examples route to reduce the noise here. This snowballed a bit since there are additional places where checking for all formats would be useful. I have a WIP branch that I'd like to pick up if it looks like the right direction.

This PR ended up being bigger than I expected, so I thought it would be best to separate additional changes and conversations.

tmr08c · 2020-05-17T22:10:19Z

spec/lib/annotate/annotate_models_spec.rb

+            end
+          end
+
+          context 'when option "format_markdown" is true' do


There didn't appear to be a test for the markdown format, so this adds a test for format_markdown and extracts some of the shared set up into befores and lets.

ctran and others added 13 commits March 10, 2015 15:44

Merge branch 'hotfix/v2.6.7'

d5d55e8

Merge branch 'release/v2.6.8'

52ffefc

Merge branch 'release/v2.6.9'

1de5bfb

Merge branch 'release/v2.6.10'

0146ee7

Runs annotate on 'rake db:rollback' too

7b33065

Merge pull request ctran#249 from agoodno/run-on-rollback

1ee47e5

Runs annotate on 'rake db:rollback' too

Revert "Runs annotate on 'rake db:rollback' too"

8f3aeb1

Merge pull request ctran#259 from ctran/revert-249-run-on-rollback

d878a5b

Revert "Runs annotate on 'rake db:rollback' too"

Merge branch 'release/v2.7.0'

0b05456

Merge branch 'release/v2.7.2'

5f37a97

Merge branch 'release/v2.7.3'

89bce57

Merge branch 'release/2.7.5'

0ef095d

Merge branch 'develop'

111640d

tmr08c mentioned this pull request Mar 24, 2020

[WIP] Fix updating annotation when formatted for Yard docs #781

Closed

5 tasks

tmr08c force-pushed the tmr08c-fix-updating-yard-doc branch 3 times, most recently from ac98ea2 to 9cd0fc2 Compare April 11, 2020 01:50

tmr08c commented Apr 11, 2020

View reviewed changes

tmr08c force-pushed the tmr08c-fix-updating-yard-doc branch from 9cd0fc2 to ecc5c9e Compare April 11, 2020 01:56

tmr08c changed the title ~~[WIP] Fix updating annotation when formatted for Yard docs~~ Fixes updating columns when using alternative formatsFixes updating columns when using alternative formats Apr 11, 2020

tmr08c marked this pull request as ready for review April 11, 2020 02:21

drwl requested changes May 8, 2020

View reviewed changes

tmr08c added 5 commits May 8, 2020 20:15

Merge remote-tracking branch 'upstream/master' into tmr08c-fix-updati…

5de6948

…ng-yard-doc

Merge remote-tracking branch 'upstream' into tmr08c-fix-updating-yard…

1cd5aa3

…-doc

Fixes updating column type for rdoc and yard

45581f5

* Adds tests for updating the type of a column * Fixes the regular expressions for `rdoc` and `yard` annotation types

Fixes updating annotation for a change in type for markdown format

f38287b

tmr08c marked this pull request as draft May 9, 2020 01:57

tmr08c commented May 9, 2020

View reviewed changes

tmr08c added 6 commits May 17, 2020 11:12

Updates column regex for bare case

5a8e84b

Updates the regular expression to more explicitly match the bare case and no longer be the general purpose regex.

Adds separate function for foreign key column regular expression

e73783c

Fix updating foreign key checks

8de5c60

- Adds markdown support (and test) - Fixes typo in `#annotate_one_file`

Renames and fixes foreign_key_column_pattern

e98a62c

- Appends `_for` to match other regex method - Fixes check in non-bareword case to not capture the `.`. This would reduce the captures being returned instead of whether the line was matched overall

Merge remote-tracking branch 'origin/develop' into tmr08c-fix-updatin…

6aef748

…g-yard-doc

Updates comments for AnnotateModels#foreign_key_column_pattern_for

4255689

tmr08c commented May 17, 2020

View reviewed changes

Removes extranesous space from annotate_models_spec

b88f55f

tmr08c marked this pull request as ready for review May 17, 2020 22:19

tmr08c requested a review from drwl May 17, 2020 22:19

tmr08c changed the title ~~Fixes updating columns when using alternative formatsFixes updating columns when using alternative formats~~ Fixes updating columns when using alternative formats Mar 24, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixes updating columns when using alternative formats #784

Fixes updating columns when using alternative formats #784

tmr08c commented Mar 24, 2020 •

edited

Loading

tmr08c Apr 11, 2020

drwl May 8, 2020

tmr08c May 9, 2020

tmr08c commented Apr 11, 2020

drwl May 8, 2020

tmr08c May 9, 2020

tmr08c May 17, 2020

tmr08c May 9, 2020

tmr08c May 9, 2020

tmr08c left a comment

tmr08c May 17, 2020

tmr08c May 17, 2020

tmr08c May 17, 2020

Fixes updating columns when using alternative formats #784

Are you sure you want to change the base?

Fixes updating columns when using alternative formats #784

Conversation

tmr08c commented Mar 24, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tmr08c commented Apr 11, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tmr08c left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tmr08c commented Mar 24, 2020 •

edited

Loading