Add option to include line number for vim formatter #65

larrylv · 2017-08-14T19:11:43Z

I was using tagbar with ripper-tags, but I couldn't jump to the tag definition because ripper-tags output doesn't have the line number information.

Not sure if this is just my need, ripper-tags might consider adding an option to support line number information. If you think this is a good feature, I could open another PR to add the -l option.

Thanks.

tmm1/ripper-tags#65

larrylv · 2017-08-14T19:25:09Z

Actually, other formatters' output all have line number information, so I think we might consider just add this as default.

mislav · 2017-08-15T10:29:30Z

This tool tries to stay as compatible as possible with ctags in its processing of input parameters and its output. Ctags by default doesn't output line numbers, and pattern matching is superior to line numbers anyway, because it allows jumping to a tag definition even in a file that you might have edited since generating the tags file.

So, let's not change the default in any way, but I would welcome a PR that implements the --excmd=number (shorthand: -n) flag for this tool.

larrylv · 2017-08-15T14:43:45Z

@mislav Thank you for your review. This tool tries to stay as compatible as possible with ctags, this makes sense to me. Changes are made, please take another look.

mislav · 2017-08-15T14:59:25Z

lib/ripper-tags.rb

@@ -53,6 +54,12 @@ def self.option_parser(options)
          options.exclude << pattern
        end
      end
+      opts.on("--excmd (number)", "Include line number information in the tag") do |pattern|


Can this say --excmd=number to match the syntax that a user would actually pass?

mislav · 2017-08-15T15:02:59Z

lib/ripper-tags/vim_formatter.rb

@@ -59,6 +59,14 @@ def display_inheritance(tag)
      end
    end

+    def display_line_number(tag)
+      if options.line_number && tag[:line]
+        "\tline:%s" % tag[:line]


Where is the line:%d syntax defined? Which types of tags processors support this field? I don't see this documented as part of "standard" tags file format: http://docs.ctags.io/en/latest/format.html

What --excmd=number does is that, instead of generating regex patterns in display_pattern, it outputs the line number in its place. If a function foo was defined on line 18, the resulting tags file would be:

foo{TAB}myfile.c{TAB}18;"{OTHER FIELDS}

I might go to the wrong direction. Actually, what makes ctags output the line number is --fields option. http://ctags.sourceforge.net/ctags.html

−−fields=[+|−]flags | n | | Line number of tag definition

So I'm thinking if we could add the option as --fields=+n for ripper-tags, it would be compatible with ctags, what do you think?

Do you mind adding support for both options, since you're already halfway done? Let's support --excmd=number as well as --fields=+n. Please process the characters within --fields with the same mechanism as --extra.

mislav · 2017-08-15T15:07:39Z

test/test_formatters.rb

@@ -63,6 +63,30 @@ def test_vim
    ))
  end

+  def test_vim_with_line_number
+    vim = formatter_for(:format => 'vim', :line_number => true)
+    assert_equal %{C\t./script.rb\t/^class C < D$/;"\tc\tline:1\tclass:A.B\tinherits:D}, vim.format(build_tag(


We don't need this many tests with variations of different tag types like class vs. module vs. method, etc. These variations are all tested in other tests. Here, we want to focus on line numbers. So instead, lets test the output for a tag that has :line => 1, then another line number :line => 18, for example, then a large line number such as :line => 123456. Nothing else other than line number needs to change in these tag definitions, to keep the tests as focused as possible.

mislav · 2017-08-15T15:07:52Z

lib/ripper-tags.rb

@@ -26,7 +26,8 @@ def self.default_options
      :files => %w[.],
      :recursive => false,
      :exclude => %w[.git],
-      :all_files => false
+      :all_files => false,
+      :line_number => false


Let's call this option plural: line_numbers

larrylv · 2017-08-15T16:12:06Z

Here is the diff with [ripper-tags|ctags] -f - --fields=+n lib/ripper-tags.rb:

➜  ~/Code/Hacks/ripper-tags ruby:(2.4.1) go:(1.8.3) git:(add-line-number-to-vim-formatter) ✔
$ ripper-tags -f - --fields=+n lib/ripper-tags.rb
!_TAG_FILE_FORMAT       2       /extended format; --format=1 will not append ;" to lines/
!_TAG_FILE_SORTED       1       /0=unsorted, 1=sorted, 2=foldcase/
FatalError      lib/ripper-tags.rb      /^  FatalError = Class.new(RuntimeError)$/;"    c       line:14 class:RipperTags        inherits:RuntimeError
RipperTags      lib/ripper-tags.rb      /^module RipperTags$/;" m       line:11
default_options lib/ripper-tags.rb      /^  def self.default_options$/;"        F       line:16 class:RipperTags
formatter_for   lib/ripper-tags.rb      /^  def self.formatter_for(options)$/;" F       line:137        class:RipperTags
option_parser   lib/ripper-tags.rb      /^  def self.option_parser(options)$/;" F       line:33 class:RipperTags
process_args    lib/ripper-tags.rb      /^  def self.process_args(argv, run = method(:run))$/;" F       line:122        class:RipperTags
run     lib/ripper-tags.rb      /^  def self.run(options)$/;"   F       line:148        class:RipperTags
version lib/ripper-tags.rb      /^  def self.version() "0.3.5" end$/;"  F       line:12 class:RipperTags
➜  ~/Code/Hacks/ripper-tags ruby:(2.4.1) go:(1.8.3) git:(add-line-number-to-vim-formatter) ✔
$ ctags -f - --fields=+n lib/ripper-tags.rb
RipperTags      lib/ripper-tags.rb      /^module RipperTags$/;" m       line:11
default_options lib/ripper-tags.rb      /^  def self.default_options$/;"        F       line:16 class:RipperTags.version
formatter_for   lib/ripper-tags.rb      /^  def self.formatter_for(options)$/;" F       line:137
option_parser   lib/ripper-tags.rb      /^  def self.option_parser(options)$/;" F       line:33 class:RipperTags.version
process_args    lib/ripper-tags.rb      /^  def self.process_args(argv, run = method(:run))$/;" F       line:122
run     lib/ripper-tags.rb      /^  def self.run(options)$/;"   F       line:148        class:formatter_for
version lib/ripper-tags.rb      /^  def self.version() "0.3.5" end$/;"  F       line:12 class:RipperTags

mislav

While you're at this, could you also add support for -n/--excmd=number? It shouldn't be hard.

mislav · 2017-08-15T16:20:03Z

lib/ripper-tags.rb

@@ -53,6 +54,9 @@ def self.option_parser(options)
          options.exclude << pattern
        end
      end
+      opts.on("--fields=+n", "Include line number information in the tag") do |value|
+        options.line_numbers = value == "+n"


This is not going to work if someone passed --fields=n or --fields=abcn (where "abc" are flags that we currently don't support). Please copy or extract the existing --extra mechanism for processing these flags and later just check if n was among them.

mislav · 2017-08-15T16:21:00Z

test/test_formatters.rb

+    assert_equal %{C\t./script.rb\t/^class C < D$/;"\tc\tline:1\tclass:A.B\tinherits:D}, vim.format(build_tag(
+      :kind => 'class', :name => 'C',
+      :pattern => "class C < D",
+      :class => 'A::B', :inherits => 'D'


None if these tests needs either :class nor :inherits since those are not the features that we're interested in here. You can take those parameters out.

larrylv · 2017-08-15T16:43:18Z

@mislav Thank you for the patient review, please take another look.

larrylv · 2017-08-15T16:49:02Z

$ .binstubs/ripper-tags -f - --excmd=number --fields=+n lib/ripper-tags.rb
!_TAG_FILE_FORMAT       2       /extended format; --format=1 will not append ;" to lines/
!_TAG_FILE_SORTED       1       /0=unsorted, 1=sorted, 2=foldcase/
FatalError      lib/ripper-tags.rb      /^  FatalError = Class.new(RuntimeError)$/;"    14;"    c       line:14 class:RipperTags        inherits:RuntimeError
RipperTags      lib/ripper-tags.rb      /^module RipperTags$/;" 11;"    m       line:11
default_options lib/ripper-tags.rb      /^  def self.default_options$/;"        16;"    F       line:16 class:RipperTags
formatter_for   lib/ripper-tags.rb      /^  def self.formatter_for(options)$/;" 156;"   F       line:156        class:RipperTags
option_parser   lib/ripper-tags.rb      /^  def self.option_parser(options)$/;" 34;"    F       line:34 class:RipperTags
process_args    lib/ripper-tags.rb      /^  def self.process_args(argv, run = method(:run))$/;" 141;"   F       line:141        class:RipperTags
run     lib/ripper-tags.rb      /^  def self.run(options)$/;"   167;"   F       line:167        class:RipperTags
version lib/ripper-tags.rb      /^  def self.version() "0.3.5" end$/;"  12;"    F       line:12 class:RipperTags
➜  ~/Code/Hacks/ripper-tags ruby:(2.4.1) go:(1.8.3) git:(add-line-number-to-vim-formatter) ✗
$ ctags -f - --excmd=number --fields=+n lib/ripper-tags.rb
RipperTags      lib/ripper-tags.rb      11;"    m       line:11
default_options lib/ripper-tags.rb      16;"    F       line:16 class:RipperTags.version
formatter_for   lib/ripper-tags.rb      156;"   F       line:156
option_parser   lib/ripper-tags.rb      34;"    F       line:34 class:RipperTags.version
process_args    lib/ripper-tags.rb      141;"   F       line:141
run     lib/ripper-tags.rb      167;"   F       line:167        class:formatter_for
version lib/ripper-tags.rb      12;"    F       line:12 class:RipperTags

mislav

Thanks for your prompt updates so far!

mislav · 2017-08-15T16:53:56Z

lib/ripper-tags.rb

+        if excmd == "number"
+          options.excmd = excmd
+        else
+          $stderr.puts "Error: excmd %p is not supported" % excmd


Let's just save any value passed to options.excmd and not raise an error on any unsupported value.

mislav · 2017-08-15T16:58:03Z

lib/ripper-tags/default_formatter.rb

+      options.fields && options.fields.include?(field)
+    end
+
+    def excmd?(excmd)


This helper is not really necessary. The caller can simply do options.excmd == 'number'

mislav · 2017-08-15T17:00:04Z

test/test_formatters.rb

+
+  def test_vim_with_excmd_number
+    vim = formatter_for(:format => 'vim', :excmd => "number")
+    assert_equal %{C\t./script.rb\t/^class C < D$/;"\t1;"\tc}, vim.format(build_tag(


The --excmd=number option enables a mode where pattern is no longer output (e.g. there is no /^class C < D$/). In its place, the line number is output.

larrylv · 2017-08-15T17:15:36Z

ptal @mislav

mislav

Looks great! One tiny nitpick.

mislav · 2017-08-15T17:26:45Z

lib/ripper-tags/vim_formatter.rb

-      tag.fetch(:pattern).to_s.gsub('\\','\\\\\\\\').gsub('/','\\/')
+    def display_excmd_info(tag)
+      if options.excmd == "number"
+        "%s;\"" % tag.fetch(:line).to_s


You can avoid to_s with

"%d;\"" % tag.fetch(:line)

addressed, thanks!

tmm1/ripper-tags#65

larrylv · 2017-08-15T22:42:21Z

👍

mislav · 2017-08-16T11:12:28Z

Thank you for your hard work!

larrylv force-pushed the add-line-number-to-vim-formatter branch from 0f173c2 to ddf6af0 Compare August 14, 2017 19:19

larrylv added a commit to larrylv/dotfiles that referenced this pull request Aug 14, 2017

Use ripper-tags for tagbar of Ruby files

98b8de7

tmm1/ripper-tags#65

larrylv added a commit to larrylv/dotfiles that referenced this pull request Aug 14, 2017

Use ripper-tags for tagbar of Ruby files

f70a69c

tmm1/ripper-tags#65

larrylv force-pushed the add-line-number-to-vim-formatter branch from ddf6af0 to 935a5d0 Compare August 15, 2017 14:42

larrylv changed the title ~~Add line number to vim formatter~~ Add option to include line number for vim formatter Aug 15, 2017

mislav requested changes Aug 15, 2017

View reviewed changes

larrylv force-pushed the add-line-number-to-vim-formatter branch from 935a5d0 to 2547b7b Compare August 15, 2017 16:08

mislav requested changes Aug 15, 2017

View reviewed changes

Add option to include line number for vim formatter

029c50b

larrylv force-pushed the add-line-number-to-vim-formatter branch from 2547b7b to 6aebc11 Compare August 15, 2017 16:42

larrylv force-pushed the add-line-number-to-vim-formatter branch from 6aebc11 to 0bf172a Compare August 15, 2017 16:48

mislav requested changes Aug 15, 2017

View reviewed changes

Support --excmd=number option

f312bc2

larrylv force-pushed the add-line-number-to-vim-formatter branch from 0bf172a to f312bc2 Compare August 15, 2017 17:15

mislav reviewed Aug 15, 2017

View reviewed changes

Avoid to_s with '%d' format

41267f3

larrylv added a commit to larrylv/dotfiles that referenced this pull request Aug 15, 2017

ripper-tags now supports line number which works for tagbar

f607488

tmm1/ripper-tags#65

mislav added 2 commits August 16, 2017 00:30

Extract command-line frags processing logic

43868ae

Extract check for supported formatter flags

742613a

mislav merged commit 78680cd into tmm1:master Aug 16, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add option to include line number for vim formatter #65

Add option to include line number for vim formatter #65

larrylv commented Aug 14, 2017

larrylv commented Aug 14, 2017

mislav commented Aug 15, 2017

larrylv commented Aug 15, 2017

mislav Aug 15, 2017

mislav Aug 15, 2017

larrylv Aug 15, 2017 •

edited

Loading

mislav Aug 15, 2017 •

edited

Loading

mislav Aug 15, 2017

larrylv Aug 15, 2017

mislav Aug 15, 2017

larrylv Aug 15, 2017

larrylv commented Aug 15, 2017

mislav left a comment

mislav Aug 15, 2017

mislav Aug 15, 2017 •

edited

Loading

larrylv commented Aug 15, 2017

larrylv commented Aug 15, 2017

mislav left a comment

mislav Aug 15, 2017

mislav Aug 15, 2017

mislav Aug 15, 2017

larrylv commented Aug 15, 2017

mislav left a comment

mislav Aug 15, 2017

larrylv Aug 15, 2017

larrylv commented Aug 15, 2017

mislav commented Aug 16, 2017

Add option to include line number for vim formatter #65

Add option to include line number for vim formatter #65

Conversation

larrylv commented Aug 14, 2017

larrylv commented Aug 14, 2017

mislav commented Aug 15, 2017

larrylv commented Aug 15, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

larrylv Aug 15, 2017 • edited Loading

Choose a reason for hiding this comment

mislav Aug 15, 2017 • edited Loading

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

larrylv commented Aug 15, 2017

mislav left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mislav Aug 15, 2017 • edited Loading

Choose a reason for hiding this comment

larrylv commented Aug 15, 2017

larrylv commented Aug 15, 2017

mislav 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

larrylv commented Aug 15, 2017

mislav left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

larrylv commented Aug 15, 2017

mislav commented Aug 16, 2017

larrylv Aug 15, 2017 •

edited

Loading

mislav Aug 15, 2017 •

edited

Loading

mislav Aug 15, 2017 •

edited

Loading