Reduce Ruby warnings further #1228

pyrmont · 2019-06-26T20:23:32Z

This commit reduces to zero all warnings generated internally by Rouge when Ruby is run with warnings enabled.

ashmaroli · 2019-06-26T21:24:27Z

Wow!! This is AWESOME, @pyrmont !! 👏 👏
Just one thought though:
"Is there a way that instance_eval(File.read(File.join(__dir__, <keyword-source>))) could be converted into a method and work as it does currently..?"

pyrmont · 2019-06-26T21:29:59Z

@ashmaroli Thanks :) You and @pocke made it a lot easier by clearing up almost all the regular expression warnings.

"Is there a way that instance_eval(File.read(File.join(__dir__, <keyword-source>))) could be converted into a method and work as it does currently..?"

I'm not sure I follow. instance_eval is a method and each call is wrapped in a memoised method. Could you explain in a bit more detail what you meant?

ashmaroli · 2019-06-26T21:37:39Z

Could you explain in a bit more detail what you meant?

I was thinking of something along the following lines:

module Rouge
  def self.get_kewords(source)
    instance_eval(File.read(File.join(__dir__, source)))
  end
end

class FooLexer < Rouge::RegexLexer
  def self.keywords
    @keywords ||= Rouge.get_keywords('foo/keywords.rb")
  end
end

But nevermind.., I tested it. Won't work..!!

jneen · 2019-06-27T00:40:21Z

lib/rouge/theme.rb

-      return @mode if arg == :absent
+      if arg == :absent
+        return (defined?(@mode) ? @mode : nil)
+      end


this seems excessive...

I agree in principle but was the simplest way I could think of to fix the warning.

Again, I'm actually completely okay with rouge depending on uninitialized instance variables. This code is basically a fancy version of ||=, which ruby explicitly supports.

I don't run Rouge with warnings turned on so I don't really care but for a library like Rouge that's typically part of a larger project (eg. Jekyll, GitLab, etc), being able to run without warnings seems like a nice feature.

Oh, and in case it helps, here's the message for this one:

lib/rouge/theme.rb:138: warning: instance variable @mode not initialized

jneen · 2019-06-27T00:41:10Z

spec/plugins/redcarpet_spec.rb

@@ -13,7 +13,7 @@
  }

  let(:markdown) {
-    Redcarpet::Markdown.new(redcarpet.new, :fenced_code_blocks => true)
+    Redcarpet::Markdown.new(redcarpet.new({}), :fenced_code_blocks => true)


??? is this not defaulted?

My best guess is that the issue here is that the class that the redcarpet object represents is defined in C directly. If you look at the documentation the implementing file is a C file and the constructor's source is a C function. I think the reference in the C function to @options is what the warning is picking up and complaining about when defined implicitly.

I'm not... actually concerned at all about implicit instance variable usage, it's a feature of ruby that I rely on quite a bit and I'm a bit miffed that ruby warns about it to be honest.

I don't think implicit instance variable usage is the problem per se. It's the way that it works with the C implementation that's causing the warning.

Although, again, that's a best guess. This is the warning:

spec/plugins/redcarpet_spec.rb:16: warning: instance variable @options not initialized

Given this is line 16, that's rather oblique:

rouge/spec/plugins/redcarpet_spec.rb

Line 16 in 904afdc

Redcarpet::Markdown.new(redcarpet.new, :fenced_code_blocks => true)

@pyrmont I think this warning should be fixed at the upstream source itself instead in the test-suite here. Besides, this warning won't affect end-users of Jekyll, Gitlab since its not part of Rouge core.

@ashmaroli The last release of Redcarpet was in December 2016. I wouldn't hold my breath on an update. We still have a circular require warning from their library that was fixed in a patch from January 2017 and that hasn't been released.

I think this was fixed in 3.5.0, so the change may no longer be needed: vmg/redcarpet@6b86656

lib/rouge/lexers/gherkin.rb

Rakefile

ashmaroli · 2019-07-02T09:46:30Z

@pyrmont Can you push the correction to the profiler_memory task separately to master and then update this branch..?
The reason I ask is comparing the memory usage on master and this PR isn't giving concrete results..

pyrmont · 2019-07-02T09:49:35Z

@ashmaroli So submit a separate PR, merge the PR and then update this branch against master? The last strep shouldn't be necessary, right? This PR already incorporates the change.

ashmaroli · 2019-07-02T09:51:45Z

You don't have to submit a PR. You can push the fix directly, can't you?
This branch should be updated after wards so that a new build is triggered on Travis

pyrmont · 2019-07-03T03:34:48Z

@ashmaroli I can push directly to master but for documentary purposes have avoided doing this. Thanks for submitting #1243 :)

ashmaroli · 2019-08-05T06:30:24Z

Conflicts galore on this branch..

This commit reduces to zero all warnings generated internally by Rouge when Ruby is run with warnings enabled.

pyrmont · 2019-08-09T17:48:35Z

MathWorks have made it so that the webpage with the MatLab functions requires JavaScript… >_<

pyrmont · 2019-08-10T00:00:26Z

This is dependent on PR #1300.

hugopeixoto · 2020-09-07T13:11:17Z

The dependency PR has been merged, so I think this can now move forward.

I can help with the rebase, but I don't think I can update this PR. Meanwhile, I'll add some review notes.

hugopeixoto · 2020-09-07T13:12:21Z

lib/rouge/lexers/elixir.rb

@@ -120,7 +120,11 @@ class Elixir < RegexLexer
              rule %r/[\\#]/, toktype
            end

-            uniq_chars = "#{open}#{close}".squeeze
+            uniq_chars = if open == close
+                           open.squeeze


I think that the point of squeeze was to attempt to remove duplicate characters, but it didn't take into account escaping.
With the condition being explicit now, both squeezes can be removed.

hugopeixoto · 2020-09-07T13:24:09Z

spec/plugins/redcarpet_spec.rb

@@ -13,7 +13,7 @@
  }

  let(:markdown) {
-    Redcarpet::Markdown.new(redcarpet.new, :fenced_code_blocks => true)
+    Redcarpet::Markdown.new(redcarpet.new({}), :fenced_code_blocks => true)


I think this was fixed in 3.5.0, so the change may no longer be needed: vmg/redcarpet@6b86656

pyrmont added the needs-review The PR needs to be reviewed label Jun 26, 2019

jneen reviewed Jun 27, 2019

View reviewed changes

ashmaroli reviewed Jun 27, 2019

View reviewed changes

lib/rouge/lexers/gherkin.rb Show resolved Hide resolved

ashmaroli reviewed Jul 2, 2019

View reviewed changes

Rakefile Outdated Show resolved Hide resolved

pyrmont added 7 commits August 9, 2019 18:17

Reduce Ruby warnings further

59766a5

This commit reduces to zero all warnings generated internally by Rouge when Ruby is run with warnings enabled.

Fix loading of Rouge version

077ac45

Return support for Ruby 2.0

dc42d39

Update autogenerated keyword files

ca6b778

Require redcarpet version with warning fixed

1e45a3d

Rewrite workarounds for theme.rb file

4f255b6

Fix bugs in various lexers

34d21ce

pyrmont force-pushed the bugfix.reduce-ruby-warnings branch from 3aa37e3 to 34d21ce Compare August 9, 2019 15:00

pyrmont added dependent-pr The PR is dependent on other changes being merged and will be reviewed later. and removed needs-review The PR needs to be reviewed labels Aug 10, 2019

pyrmont closed this Jan 8, 2020

pyrmont deleted the bugfix.reduce-ruby-warnings branch January 8, 2020 20:08

pyrmont restored the bugfix.reduce-ruby-warnings branch January 8, 2020 20:12

pyrmont reopened this Jan 8, 2020

hugopeixoto reviewed Sep 7, 2020

View reviewed changes

tancnle added needs-review The PR needs to be reviewed and removed dependent-pr The PR is dependent on other changes being merged and will be reviewed later. labels May 17, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Reduce Ruby warnings further #1228

Reduce Ruby warnings further #1228

pyrmont commented Jun 26, 2019

ashmaroli commented Jun 26, 2019

pyrmont commented Jun 26, 2019

ashmaroli commented Jun 26, 2019

jneen Jun 27, 2019

pyrmont Jun 27, 2019

jneen Jun 27, 2019

pyrmont Jun 27, 2019

pyrmont Jun 27, 2019 •

edited

Loading

jneen Jun 27, 2019

pyrmont Jun 27, 2019 •

edited

Loading

jneen Jun 27, 2019

pyrmont Jun 27, 2019

pyrmont Jun 27, 2019 •

edited

Loading

ashmaroli Jun 27, 2019

pyrmont Jun 27, 2019

hugopeixoto Sep 7, 2020

ashmaroli commented Jul 2, 2019

pyrmont commented Jul 2, 2019

ashmaroli commented Jul 2, 2019

pyrmont commented Jul 3, 2019

ashmaroli commented Aug 5, 2019

pyrmont commented Aug 9, 2019

pyrmont commented Aug 10, 2019

hugopeixoto commented Sep 7, 2020

hugopeixoto Sep 7, 2020

hugopeixoto Sep 7, 2020

Reduce Ruby warnings further #1228

Are you sure you want to change the base?

Reduce Ruby warnings further #1228

Conversation

pyrmont commented Jun 26, 2019

ashmaroli commented Jun 26, 2019

pyrmont commented Jun 26, 2019

ashmaroli commented Jun 26, 2019

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

pyrmont Jun 27, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pyrmont Jun 27, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pyrmont Jun 27, 2019 • 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

ashmaroli commented Jul 2, 2019

pyrmont commented Jul 2, 2019

ashmaroli commented Jul 2, 2019

pyrmont commented Jul 3, 2019

ashmaroli commented Aug 5, 2019

pyrmont commented Aug 9, 2019

pyrmont commented Aug 10, 2019

hugopeixoto commented Sep 7, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pyrmont Jun 27, 2019 •

edited

Loading

pyrmont Jun 27, 2019 •

edited

Loading

pyrmont Jun 27, 2019 •

edited

Loading