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.

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

rubocop: Fix Style cops #180

Merged
merged 1 commit into from
Jun 9, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
3 changes: 0 additions & 3 deletions .rubocop.yml
Original file line number Diff line number Diff line change
Expand Up @@ -4,9 +4,6 @@ inherit_from: .rubocop_todo.yml
inherit_gem:
voxpupuli-rubocop: rubocop.yml

Style:
Enabled: false

Layout:
Enabled: false

Expand Down
61 changes: 59 additions & 2 deletions .rubocop_todo.yml
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
# This configuration was generated by
# `rubocop --auto-gen-config`
# on 2024-06-09 13:09:14 UTC using RuboCop version 1.63.5.
# on 2024-06-09 17:48:43 UTC using RuboCop version 1.63.5.
# The point is for the user to remove these configuration records
# one by one as the offenses are removed from the code base.
# Note that changes in the inspected code, or installation of new
Expand Down Expand Up @@ -52,7 +52,7 @@ RSpec/MessageSpies:
RSpec/MultipleExpectations:
Max: 2

# Offense count: 49
# Offense count: 40
# Configuration parameters: EnforcedStyle, IgnoreSharedExamples.
# SupportedStyles: always, named_only
RSpec/NamedSubject:
Expand All @@ -74,3 +74,60 @@ RSpec/StubbedMock:
Rake/Desc:
Exclude:
- 'Rakefile'

# Offense count: 6
# This cop supports unsafe autocorrection (--autocorrect-all).
# Configuration parameters: EnforcedStyle.
# SupportedStyles: always, always_true, never
Style/FrozenStringLiteralComment:
Exclude:
- 'Gemfile'
- 'Rakefile'
- 'lib/rspec-puppet-facts.rb'
- 'lib/rspec-puppet-facts/version.rb'
- 'spec/rspec_puppet_facts_spec.rb'
- 'spec/spec_helper.rb'

# Offense count: 1
Style/MixinUsage:
Exclude:
- 'spec/spec_helper.rb'

# Offense count: 3
# This cop supports unsafe autocorrection (--autocorrect-all).
# Configuration parameters: EnforcedStyle.
# SupportedStyles: literals, strict
Style/MutableConstant:
Exclude:
- 'lib/rspec-puppet-facts.rb'
- 'lib/rspec-puppet-facts/version.rb'
- 'spec/rspec_puppet_facts_spec.rb'

# Offense count: 1
# This cop supports unsafe autocorrection (--autocorrect-all).
# Configuration parameters: Methods.
Style/RedundantArgument:
Exclude:
- 'lib/rspec-puppet-facts.rb'

# Offense count: 1
# This cop supports unsafe autocorrection (--autocorrect-all).
Style/RedundantSort:
Copy link
Member

Choose a reason for hiding this comment

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

Doesn't #181 solve that?

Exclude:
- 'lib/rspec-puppet-facts.rb'

# Offense count: 2
# This cop supports unsafe autocorrection (--autocorrect-all).
# Configuration parameters: ConvertCodeThatCanStartToReturnNil, AllowedMethods, MaxChainLength.
# AllowedMethods: present?, blank?, presence, try, try!
Style/SafeNavigation:
Exclude:
- 'lib/rspec-puppet-facts.rb'

# Offense count: 2
# This cop supports unsafe autocorrection (--autocorrect-all).
# Configuration parameters: RequireEnglish, EnforcedStyle.
# SupportedStyles: use_perl_names, use_english_names, use_builtin_english_names
Style/SpecialGlobalVars:
Exclude:
- 'lib/rspec-puppet-facts.rb'
6 changes: 3 additions & 3 deletions Gemfile
Original file line number Diff line number Diff line change
Expand Up @@ -2,14 +2,14 @@ source ENV['GEM_SOURCE'] || 'https://rubygems.org'

gemspec

gem 'facter', ENV['FACTER_GEM_VERSION'], :require => false
gem 'facter', ENV.fetch('FACTER_GEM_VERSION', nil), require: false

group :release do
gem 'faraday-retry', '~> 2.1', require: false
gem 'github_changelog_generator', '~> 1.16.4', require: false
end

group :coverage, optional: ENV['COVERAGE']!='yes' do
gem 'codecov', :require => false
gem 'simplecov-console', :require => false
gem 'codecov', require: false
gem 'simplecov-console', require: false
end
6 changes: 3 additions & 3 deletions Rakefile
Original file line number Diff line number Diff line change
Expand Up @@ -40,8 +40,8 @@ namespace :puppet_versions do

output = `git status --porcelain #{PUPPET_VERSIONS_PATH}`
unless output.strip.empty?
$stderr.puts "#{PUPPET_VERSIONS_PATH} is out of date."
$stderr.puts 'Run the puppet_versions:update task to update it and commit the changes.'
warn "#{PUPPET_VERSIONS_PATH} is out of date."
warn 'Run the puppet_versions:update task to update it and commit the changes.'
raise
end
end
Expand All @@ -54,7 +54,7 @@ rescue LoadError
else
require 'rubygems'
GitHubChangelogGenerator::RakeTask.new :changelog do |config|
config.exclude_labels = %w{duplicate question invalid wontfix wont-fix skip-changelog github_actions}
config.exclude_labels = %w[duplicate question invalid wontfix wont-fix skip-changelog github_actions]
config.user = 'voxpupuli'
config.project = 'rspec-puppet-facts'
gem_version = Gem::Specification.load("#{config.project}.gemspec").version
Expand Down
41 changes: 15 additions & 26 deletions lib/rspec-puppet-facts.rb
Original file line number Diff line number Diff line change
Expand Up @@ -90,7 +90,7 @@ def on_supported_os_implementation(opts = {})
os_release_filter = "\"#{operatingsystemmajrelease}\""
when /Amazon/i
# Tighten the regex for Amazon Linux 2 so that we don't pick up Amazon Linux 2016 or 2017 facts
os_release_filter = "/^2$/" if operatingsystemmajrelease == '2'
os_release_filter = '/^2$/' if operatingsystemmajrelease == '2'
end

filter << {
Expand All @@ -110,9 +110,9 @@ def on_supported_os_implementation(opts = {})
end
end

strict_requirement = RspecPuppetFacts::facter_version_to_strict_requirement(facterversion)
strict_requirement = RspecPuppetFacts.facter_version_to_strict_requirement(facterversion)

loose_requirement = RspecPuppetFacts::facter_version_to_loose_requirement(facterversion)
loose_requirement = RspecPuppetFacts.facter_version_to_loose_requirement(facterversion)
received_facts = []

# FacterDB may have newer versions of facter data for which it contains a subset of all possible
Expand All @@ -127,9 +127,7 @@ def on_supported_os_implementation(opts = {})
version, facts = versions.select { |v, _f| loose_requirement =~ v }.max_by { |v, _f| v } if loose_requirement
next unless version

if RspecPuppetFacts.spec_facts_strict?
raise ArgumentError, "No facts were found in the FacterDB for Facter v#{facterversion} on #{filter_spec}, aborting"
end
raise ArgumentError, "No facts were found in the FacterDB for Facter v#{facterversion} on #{filter_spec}, aborting" if RspecPuppetFacts.spec_facts_strict?

RspecPuppetFacts.warning "No facts were found in the FacterDB for Facter v#{facterversion} on #{filter_spec}, using v#{version} instead"
end
Expand Down Expand Up @@ -193,7 +191,7 @@ def add_custom_fact(name, value, options = {})
def self.register_custom_fact(name, value, options)
@custom_facts ||= {}
name = RSpec.configuration.facterdb_string_keys ? name.to_s : name.to_sym
@custom_facts[name] = {:options => options, :value => value}
@custom_facts[name] = {options: options, value: value}
end

# Adds any custom facts according to the rules defined for the operating
Expand Down Expand Up @@ -235,7 +233,7 @@ def self.custom_facts
# @return [nil,String]
# @api private
def self.spec_facts_os_filter
ENV['SPEC_FACTS_OS']
ENV.fetch('SPEC_FACTS_OS', nil)
end

# If SPEC_FACTS_STRICT is set to `yes`, RspecPuppetFacts will error on missing FacterDB entries, instead of warning & skipping the tests, or using an older facter version.
Expand All @@ -252,16 +250,14 @@ def self.spec_facts_strict?
def self.common_facts
return @common_facts if @common_facts
@common_facts = {
:puppetversion => Puppet.version,
:rubysitedir => RbConfig::CONFIG['sitelibdir'],
:rubyversion => RUBY_VERSION,
puppetversion: Puppet.version,
rubysitedir: RbConfig::CONFIG['sitelibdir'],
rubyversion: RUBY_VERSION,
}

@common_facts[:mco_version] = MCollective::VERSION if mcollective?

if augeas?
@common_facts[:augeasversion] = Augeas.open(nil, nil, Augeas::NO_MODL_AUTOLOAD).get('/augeas/version')
end
@common_facts[:augeasversion] = Augeas.open(nil, nil, Augeas::NO_MODL_AUTOLOAD).get('/augeas/version') if augeas?
Copy link
Member

Choose a reason for hiding this comment

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

I'm not happy with the condition given the line length. AFAIK the guard condition cop supports line lengths, but we disabled that cop so the guard condition cop is too trigger happy.

Copy link
Member Author

Choose a reason for hiding this comment

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

@common_facts = stringify_keys(@common_facts) if RSpec.configuration.facterdb_string_keys

@common_facts
Expand Down Expand Up @@ -298,9 +294,7 @@ def self.mcollective?
# @return [Array<Hash>]
# @api private
def self.meta_supported_os
unless metadata['operatingsystem_support'].is_a? Array
fail StandardError, 'Unknown operatingsystem support in the metadata file!'
end
raise StandardError, 'Unknown operatingsystem support in the metadata file!' unless metadata['operatingsystem_support'].is_a? Array
Copy link
Member

Choose a reason for hiding this comment

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

I'd say this is less readable. Can we disable the guard condition cop?

Copy link
Member Author

Choose a reason for hiding this comment

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

metadata['operatingsystem_support']
end

Expand All @@ -311,9 +305,7 @@ def self.meta_supported_os
# @api private
def self.metadata
return @metadata if @metadata
unless File.file? metadata_file
fail StandardError, "Can't find metadata.json... dunno why"
end
raise StandardError, "Can't find metadata.json... dunno why" unless File.file? metadata_file
Copy link
Member

Choose a reason for hiding this comment

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

Same here

Copy link
Member Author

Choose a reason for hiding this comment

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

content = File.read metadata_file
@metadata = JSON.parse content
end
Expand All @@ -329,7 +321,7 @@ def self.metadata_file
# @param message [String]
# @api private
def self.warning(message)
STDERR.puts message
warn message
end

# Reset the memoization
Expand Down Expand Up @@ -379,9 +371,6 @@ def self.facter_version_to_loose_requirement_string(version)
elsif /\A[0-9]+\Z/.match?(version)
# Interpret 3 as < 4
"< #{version.to_i + 1}"
else
# This would be the same as the strict requirement
Copy link
Member

Choose a reason for hiding this comment

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

I originally wrote this because it explicitly documents the flow. Is it worth keeping that?

nil
end
end

Expand All @@ -397,13 +386,13 @@ def self.facter_version_for_puppet_version(puppet_version)
fd = File.open(json_path, 'rb:UTF-8')
data = JSON.parse(fd.read)

version_map = data.map { |_, versions|
version_map = data.map do |_, versions|
Copy link
Member

Choose a reason for hiding this comment

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

In #183 I do more refactoring that I think helps. I'd expect .map.compact to be flagged in rubocop-performance.

if versions['puppet'].nil? || versions['facter'].nil?
nil
else
[Gem::Version.new(versions['puppet']), versions['facter']]
end
}.compact
end.compact

puppet_gem_version = Gem::Version.new(puppet_version)
applicable_versions = version_map.select { |p, _| puppet_gem_version >= p }
Expand Down
Loading