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

Fixes the case, when config_store is nil #24

Closed
wants to merge 2 commits into from

Conversation

prog-supdex
Copy link
Contributor

This problem appeared when rubocop-md loaded via .rubocop.yml and used --formater because the Runner is created before plugins (rubocop_ext) is loaded.

Fixes #22

Also, I rewrote the running test through the require section in .rubocop.yml.

This problem appeared when "rubocop-md" loaded via .rubocop.yml and used "--formater"

Fixes rubocop#22
@prog-supdex prog-supdex changed the title Fixes #22 Fixes the case, when config_store is nil. #22 Oct 18, 2023
@prog-supdex prog-supdex changed the title Fixes the case, when config_store is nil. #22 Fixes the case, when config_store is nil Oct 18, 2023
Copy link
Collaborator

@palkan palkan left a comment

Choose a reason for hiding this comment

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

Left some feedback; please, read through. Merged via #25

Comment on lines +21 to +22
Rake::Task[:rubocop_md_tests].reenable
Rake::Task[:rubocop_md_tests].invoke
Copy link
Collaborator

Choose a reason for hiding this comment

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

We should define individual tasks and then an aggregate using Rake dependencies.

The main reason is that we need a way to run each mode individually.

@@ -11,4 +11,15 @@ end

RuboCop::RakeTask.new

task :test do
ENV["MD_LOAD_MODE"] = "inline"
$stdout.puts "⚙️ Runs rubocop with '-r rubocop_md' options"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Adding notifications is good idea, but no in the Rakefile. It should be a part of the test suite, so it doesn't depend on the way we run tests (via Rake, via Ruby, or whatever). Having notification here can result in false positives when the actual test suite is not using this env var.

@@ -2,7 +2,7 @@ require "bundler/gem_tasks"
require "rake/testtask"
require "rubocop/rake_task"

Rake::TestTask.new(:test) do |t|
Rake::TestTask.new(:rubocop_md_tests) do |t|
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why prefixing with rubocop_md? This is the Rakefile for the project, so theme is implied.

Comment on lines +21 to 33
def cmd_command_by_env(path, options)
cmd_command = "bundle exec rubocop"
load_mode = ENV.fetch("MD_LOAD_MODE", MD_LOAD_INLINE_MODE)

if load_mode == MD_LOAD_INLINE_MODE
md_path = File.expand_path("../lib/rubocop-md.rb", __dir__)

cmd_command = "#{cmd_command} -r #{md_path}"
end

"#{cmd_command} #{options} #{path}"
end
end
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is an example of improper usage of the extract method pattern. It breaks cohesion; previously, the code related to the command generation was in a single place; now, we have it spread across the file and we lose the context (what is -r, where it comes from)?

Longer methods are sometimes (quite often) better.

@palkan palkan closed this in #25 Oct 21, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

"undefined method `for' for nil:NilClass" occurs when the format option is specified
2 participants