Backport commits from ruby/ruby #189
Conversation
The compliation-mode of Emacs sets TERM to "dumb" and does not support coloring.
schneems
left a comment
schneems
left a comment
There was a problem hiding this comment.
One question, no blockers. Feel free to merge.
| script.write(contents) | ||
|
|
||
| out = `#{ruby} -I#{lib_dir} -rsyntax_suggest #{script} 2>&1` | ||
| out = `#{ruby} -I#{lib_dir} -rsyntax_suggest/version #{script} 2>&1` |
There was a problem hiding this comment.
Can you give some context for why this change was made? Is it a performance issue or something else?
I’m good with the PR overall but want to preserve some context so if someone refactors this later they will understand the intent.
There was a problem hiding this comment.
It's not performance reason.
-rsyntax_suggest didn't work with make test-syntax-suggest on ruby/ruby repository. It raises uninitialized constant SyntaxSuggest::VERSION (NameError). I'm not sure why they are different result.
There was a problem hiding this comment.
-rsyntax_suggestdidn't work withmake test-syntax-suggeston ruby/ruby repository. It raisesuninitialized constant SyntaxSuggest::VERSION (NameError). I'm not sure why they are different result.
I believe because the lib by default doesn't require version file: https://github.com/ruby/syntax_suggest/blob/b57fe7f/lib/syntax_suggest.rb
I think it's OK practice. We can do in this way, or left it here and just add require_relative 'syntax_suggest/version' in the main lib file.
Nothing of this is performance-related, I think.
Also you can think in this way: if you want to inspect a library, or even a project with this library, where it's required, and you faced some issues, and want to check its version — SyntaxSuggest::VERSION theoretically exist, but there (in irb or pry) also will be the exception without additional require 'syntax_suggest/version'.
There was a problem hiding this comment.
I think originally I did that as a kind of guard for if the code was loaded or not. But I’m using this now
so if we want to load the version always I think it’s no problem
There was a problem hiding this comment.
Maybe we should use something like autoload?
There was a problem hiding this comment.
I prefer to load the version file instead of autoload.
I remember hitting some edgecase-y things with autoload when working on this. Unfortunately I didn’t write down exactly what those problems were.
There was a problem hiding this comment.
Yeah, I think simple require_relative with single VERSION constant would not make a big impact on performance.
I picked tweak commits from https://github.com/ruby/ruby