Skip to content

Backport commits from ruby/ruby #189

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
hsbt merged 4 commits into from
Apr 17, 2023
Merged

Backport commits from ruby/ruby #189

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
38062b3
Disable color mode of test-syntax-suggest when on dumb terminal
nobu Mar 8, 2023
0f795e7
Fix commit miss
nobu Mar 8, 2023
a9cda1a
Load only SyntaxSuggest::VERSION for version check
hsbt Apr 6, 2023
6e266b3
standardrb --fix-unsafely spec/spec_helper.rb
hsbt Apr 6, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion spec/integration/ruby_command_line_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,7 +58,7 @@ module SyntaxSuggest
EOM
script.write(contents)

out = `#{ruby} -I#{lib_dir} -rsyntax_suggest #{script} 2>&1`
out = `#{ruby} -I#{lib_dir} -rsyntax_suggest/version #{script} 2>&1`
Copy link
Collaborator

@schneems schneems Apr 7, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

@hsbt hsbt Apr 10, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link

@AlexWayfer AlexWayfer Apr 10, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

-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.

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'.

Copy link
Collaborator

@schneems schneems Apr 10, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

syntax_suggest/lib/syntax_suggest/core_ext.rb

Line 25 in b57fe7f

require "syntax_suggest/api" unless defined?(SyntaxSuggest::DEFAULT_VALUE)

so if we want to load the version always I think it’s no problem

Copy link

@AlexWayfer AlexWayfer Apr 10, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe we should use something like autoload?

Copy link
Collaborator

@schneems schneems Apr 12, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link

@AlexWayfer AlexWayfer Apr 12, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I think simple require_relative with single VERSION constant would not make a big impact on performance.


expect(out).to include("suggest_version is #{SyntaxSuggest::VERSION}").once
end
Expand Down
6 changes: 6 additions & 0 deletions spec/spec_helper.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,12 @@
config.expect_with :rspec do |c|
c.syntax = :expect
end

if config.color_mode == :automatic
if config.color_enabled? && ((ENV["TERM"] == "dumb") || ENV["NO_COLOR"]&.slice(0))
config.color_mode = :off
end
end
end

# Used for debugging modifications to
Expand Down