Skip to content
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

Jump to bottom

Normalize heredoc delimiters #3533

Merged
merged 2 commits into from
Apr 23, 2020
Merged

Normalize heredoc delimiters #3533

merged 2 commits into from
Apr 23, 2020

Conversation

deivid-rodriguez
Copy link
Member

Description:

On another PR, I noticed that while normally we use RUBY as the delimiter for heredocs including ruby code, sometimes we use RB.

RUBY is better so I'm replacing RB occurrences with RUBY.

Tasks:

  • Describe the problem / feature
  • Write tests
  • Write code to solve the problem
  • Get code review from coworkers / friends

I will abide by the code of conduct.

@simi
Copy link
Member

simi commented Apr 20, 2020

🤔 I see there are various HEREDOC names used across the code. Some are lower case, some upper case, some have meaningful name like RUBY, some have less meaningful name like D or EOF.

There are two related cops we can consider to use as well if you're doing HEREDOC cleanup.

https://rubocop.readthedocs.io/en/latest/cops_naming/#namingheredocdelimitercase
https://rubocop.readthedocs.io/en/latest/cops_naming/#namingheredocdelimiternaming

@deivid-rodriguez
Copy link
Member Author

Good point @simi. I enabled Naming/HeredocLimiterNaming to check for the specific issue I wanted to normalize, and also enabled the delimiter case cop.

@deivid-rodriguez
Copy link
Member Author

@simi You ok with this?

simi
simi reviewed Apr 23, 2020
View reviewed changes
lib/rubygems.rb
@@ -401,11 +401,11 @@ def self.paths=(env)
target[k] = v
when Array
unless Gem::Deprecate.skip
warn <<-eowarn
warn <<-EOWARN
Copy link
Member

Choose a reason for hiding this comment

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

Can we unify this to use WARNING as well?

simi
simi reviewed Apr 23, 2020
View reviewed changes
test/rubygems/test_gem_ext_cmake_builder.rb
@@ -23,11 +23,11 @@ def setup

def test_self_build
File.open File.join(@ext, 'CMakeLists.txt'), 'w' do |cmakelists|
cmakelists.write <<-eo_cmake
cmakelists.write <<-EO_CMAKE
Copy link
Member

Choose a reason for hiding this comment

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

Would CMAKE be enough here?

simi
simi reviewed Apr 23, 2020
View reviewed changes
test/rubygems/test_gem_remote_fetcher.rb
@@ -81,7 +81,7 @@ class TestGemRemoteFetcher < Gem::TestCase
# Generated via:
# x = OpenSSL::PKey::DH.new(2048) # wait a while...
# x.to_s => pem
TEST_KEY_DH2048 = OpenSSL::PKey::DH.new <<-_end_of_pem_
TEST_KEY_DH2048 = OpenSSL::PKey::DH.new <<-_END_OF_PEM_
Copy link
Member

Choose a reason for hiding this comment

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

PEM?

simi
simi reviewed Apr 23, 2020
View reviewed changes
test/rubygems/test_gem_request_set.rb
@@ -396,9 +396,9 @@ def test_resolve_git
rs = Gem::RequestSet.new

tf = Tempfile.open 'gem.deps.rb' do |io|
io.puts <<-gems_deps_rb
io.puts <<-GEMS_DEPS_RB
Copy link
Member

Choose a reason for hiding this comment

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

If I understand it well, this is also just code ruby block. Any reason to not use RUBY as well?

simi
simi reviewed Apr 23, 2020
View reviewed changes
test/rubygems/test_gem_commands_setup_command.rb
@@ -359,7 +359,7 @@ def test_show_release_notes
@cmd.options[:previous_version] = Gem::Version.new '2.0.2'

File.open 'History.txt', 'w' do |io|
io.puts <<-History_txt
io.puts <<-HISTORY_TXT
Copy link
Member

Choose a reason for hiding this comment

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

Would TEXT be enough here?

Copy link
Member

Choose a reason for hiding this comment

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

Also can't we use

File.write('History.txt', <<-TEXT)
  ...
TEXT

here?

@simi
Copy link
Member

simi commented Apr 23, 2020

@deivid-rodriguez I left some minor non-blocking comments from my side.

@deivid-rodriguez
Copy link
Member Author

@simi I only made the necessary changes to get the Naming/HeredocDelimiterCase cop green since you mentioned it, but I didn't intend to make a whole review of heredoc naming. There's for example a lot of heredocs named G, which could be renamed to GEMFILE or RUBY as well. So much stuff that can be improved in the code base...

Let me get this in and iterate on your suggestions next time I work on a style PR, ok? :)

simi
simi approved these changes Apr 23, 2020
View reviewed changes
Copy link
Member

@simi simi left a comment

Choose a reason for hiding this comment

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

Sure, good job anyway!

@deivid-rodriguez deivid-rodriguez merged commit 498da08 into master Apr 23, 2020
@deivid-rodriguez deivid-rodriguez deleted the rb_vs_ruby branch April 23, 2020 19:53
@Irakaj93 Irakaj93 mentioned this pull request Dec 26, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@simi simi simi approved these changes

Assignees
No one assigned
Labels
Bundler
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@deivid-rodriguez @simi @bundlerbot