Added the Ability to Generate a todo File

[mrbiggred]

Useful if you have an existing project that you want to incorporate Standard into. Run the below to generate the .standard_todo.yml file:

standardrb --gen-ignore

I thought it would be better to have a separate ignore file. Easier to generate. Also allows developers to separate the files that are "temporarily" ignored versus the files to permanently ignore in the .standard.yml.

Looking forward to your feedback. Thank you

[mrbiggred]

It appears the "cleanpath" error is a known Ruby issue:

https://bugs.ruby-lang.org/issues/10011

I didn't encounter this issue as I used Ruby 2.6 when testing. I'll update my code so it works on Ruby 2.4.1, which is what the tests appear to run on.

The second error with the path names not lining up is interesting. I think something is bleeding over between the the new genignore_test.rb tests I added and the existing rubocop_test.rb tests.

bundle exec rake test
Run options: --seed 61415

E..F............................................................

Finished in 4.444038s, 14.4013 runs/s, 23.1771 assertions/s.

1. Error:
   Standard::Runners::GenignoreTest#test_todo_generated:
   NoMethodError: undefined method cleanpath' for "/home/circleci/repo/tmp/genignore_test":String /usr/local/lib/ruby/2.4.0/pathname.rb:506:in relative_path_from'
   /home/circleci/repo/lib/standard/runners/genignore.rb:19:in block in call' /home/circleci/repo/lib/standard/runners/genignore.rb:18:in map!'
   /home/circleci/repo/lib/standard/runners/genignore.rb:18:in call' /home/circleci/repo/test/standard/runners/genignore_test.rb:19:in block in test_todo_generated'
   /home/circleci/repo/test/standard/runners/genignore_test.rb:18:in chdir' /home/circleci/repo/test/standard/runners/genignore_test.rb:18:in test_todo_generated'

2. Failure:
   Standard::Runners::RubocopTest#test_print_corrected_output_on_stdin [/home/circleci/repo/test/standard/runners/rubocop_test.rb:66]:
   --- expected
   +++ actual
   @@ -1,4 +1,4 @@
   -"== test/fixture/runner/agreeable.rb ==
   +"== /home/circleci/repo/test/fixture/runner/agreeable.rb ==
   " +
   "C: 1: 1: [Corrected] Style/FrozenStringLiteralComment: Missing magic comment # frozen_string_literal: true.
   " +

64 runs, 103 assertions, 1 failures, 1 errors, 0 skips
Coverage report generated for Unit Tests to /home/circleci/repo/coverage. 763 / 782 LOC (97.57%) covered.
rake aborted!
Command failed with status (1)
/home/circleci/repo/vendor/bundle/ruby/2.4.0/gems/rake-12.3.2/exe/rake:27:in <top (required)>' /usr/local/bin/bundle:23:in load'
/usr/local/bin/bundle:23:in `

'
Tasks: TOP => test
(See full trace by running task with --trace)

Exited with code exit status 1

[mrbiggred]

Sorry but I won't be able to fix the above issues till after the holidays. Let me know having a PR open for that long will cause any issues and we can figure something out, like closing this one and re-submitting a new one in the new year.

Thanks and Happy Holidays!

[mrbiggred]

The error with Ruby 2.4.1 and relative_path_from is fixed. Still can't figure out why the path is incorrect if my new runner tests runs before the existing Rubocop runner test but I'll keep poking around till I find the problem.

[searls]

Thanks for your continued efforts!

[mrbiggred]

Just a heads up that I plan on taking another look at the failing tests this Thursday. It appears that the underlying Rubocop settings are persisted between the tests. At least that is my current theory. Does anyone has any suggestions on how to clear the Rubocop settings between tests? Thanks.

[mrbiggred]

I have finally found and fixed the failing test issue. The problem was Rubocop was caching the working directory. This combined with how simple formatter works would cause the test_print_corrected_output_on_stdin in test/standard/runners/rubocop_test to fail if it ran before the new test_todo_generated test I added in test/standard/runners/genignore_test.

Note: the quiet formatter inherits from simple formatter so will have the same problem.

To fix the problem I added a Rubcop path reset to test/test_helper:

def setup
  RuboCop::PathUtil.reset_pwd
end

I then changed the test/standard/runners/rubocop_test and test/standard/runners/genignore_test setup methods to use the base setup:

def setup
  super

  @subject = Standard::Runners::Genignore.new
end

Let me know if you can think of a better was to fix this failing test issue.

Debugging Notes
The code where Rubocop caches the current working directory can be found below:

https://github.com/rubocop-hq/rubocop/blob/31fe1a58d196f474991daf64d97fe64e9836bb71/lib/rubocop/path_util.rb#L57

This is the commit that added the caching to Rubocop:

https://github.com/rubocop-hq/rubocop/pull/5137/files

A description of how simple formatter with either display the relative or absolute path can be found below. I couldn't find anywhere on the main Rubocop documentation that describes how simple formatter decides the if it should use relative or absolute paths.

rubocop/rubocop#262 (comment)

The simple formatter code uses the smart_path method to output the file name. The smart_path method will use the cached working directory:

https://github.com/rubocop-hq/rubocop/blob/31fe1a58d196f474991daf64d97fe64e9836bb71/lib/rubocop/formatter/simple_text_formatter.rb#L41

[searls]

I just took a quick pass at this and it seems mostly fine. I still need to pull it down, run the tests, and play with it but wanted to give you a chance to address these bits first

[mrbiggred]

@searls thank you for your feedback. I've made the changes you suggested. Let me know what you think.

[searls]

Ignorant comment for still having not pulled down the code to run it:

Does standard print a warning when it's running in "todo" mode? I'm ok with naming it "todo" only if that's the case. Come to think of it, printing a progress-to-completion whenever a todo ignore was present would be pretty great.

What I want to avoid is anyone using this as a crutch to think that they're "passing" standard when the presence of a file is actually giving them a free pass

[mrbiggred]

@searls if I understand the above correctly you would like Standard to notify the user that the todo file is being used. Currently it does not but I like that idea.

Something like:

$ bundle exec standardrb

Todo file found, ignoring the following files:
- file_a.rb
- file_b.rb

Let me know if I misunderstood what you are asking for. Thanks.

[searls]

Yes, something like this would be good. I might make it slightly more scary:

WARNING: this project is being migrated to standard gradually via `whatevertodo.yml` and is ignoring these files:

[searls]

Howdy @mrbiggred -- do you have what you need from me to get this over the finish line, you think?

[mrbiggred]

@searls thank you for reminding me about this pull request. With all the coronavirus craziness I forgot about it. I took a look this morning and think I see where I would add the message and am working on getting a list of the files ignored. I'll try to continue my work later this week but if you don't hear from me by next week don't be afraid to ping me again.

[mrbiggred]

@searls I have added the warning message if the todo file is being used. I struggled to figure out the best way to store and print out the files being ignored. If you notice a better way please let me know.

The merge conflicts are a result of standard warning me about missing trailing commas.

I will try this branch on an actual product after the long weekend for some beta testing.

[searls]

What's exclude.txt? It's only mentioned here.

As for temp_exclude.txt, I don't love spitting out a temporary file into a person's project directory, because if there's a failure it'll sit there and they won't know what it is. Maybe it'd be better to use Ruby's tempfile instead?

[mrbiggred]

I've updated to the code to use Ruby's tempfile. Thanks for the tip.

[bhaibel]

Seagulling in to express how excited I am about this PR -- I'll feel a lot more able to use Standard in my existing projects once it lands. Happy to help get it over the finish line if there's anything I can do!

[searls]

@bhaibel thanks for the bump. I'll try to look at this tomorrow

[searls]

Hey @mrbiggred, I just merged in master and can't get the build to pass, with this failure:

Standard::Runners::GenignoreTest#test_todo_generated [/home/circleci/repo/test/standard/runners/genignore_test.rb:25]:
--- expected
+++ actual
@@ -1 +1 @@
-{"ignore"=>["errors_one.rb", "errors_two.rb"]}
+{"ignore"=>[]}

Could you take a look at it?

[mrbiggred]

@searls I'll investigate the error you are getting when trying to merge to master this morning and let you know what I find.

[mrbiggred]

@searls I finally fixed the problem but I'm not sure the exact root cause. The problem appears to be the Rubocop Config Store settings are leaking between tests. If I run the failing tests all by it's self it works. If the BuildConfigTests run before the GenignoreTests then the then GenignoreTests will fail.

It appears that Rubocop is somehow caching the Config Store settings. Specifically the exclude settings to ignore the tmp/**/* files in the below:

# test/standard/runners/genignore_test.rb

def config_store(config_root = nil, rubocop_yml = "config/base.yml", ruby_version = RUBY_VERSION)
  RuboCop::ConfigStore.new.tap do |config_store|
    config_store.options_config = path(rubocop_yml)
    options_config = config_store.instance_variable_get("@options_config")
    options_config["AllCops"]["TargetRubyVersion"] = ruby_version.to_f
    options_config["AllCops"]["Exclude"] |= standard_default_ignores(config_root)
  end.for("").to_h
end

Since the GenignoreTests puts it's test files in the tmp folder they would be ignored. To fix the problem I changed the following:

# test/standard/runners/genignore_test.rb

def create_config
  Standard::Config.new(nil, ["."], {}, Rubocop::ConfigStore.new)
end

to:

# test/standard/runners/genignore_test.rb

def create_config(config_path)
  store = RuboCop::ConfigStore.new.tap do |config_store|
    config_store.options_config = config_path
  end

  Standard::Config.new(nil, ["."], {}, store)
end

That resets the Config Store so the the tmp folder is no longer excludes. If you have any insight into why this occurred and/or a better way to fix the test let me know.

[searls]

@mrbiggred I'm happy to let the mystery be, as it were

[searls]

Landed in 0.4.0! Thanks so much for your work on this and so that @bhaibel can use it!

I officially nominate you both to long-term maintenance of this feature because I don't suspect I'll ever use it 😄

[mrbiggred]

@searls thanks for accepting my PR. I'd be happy to fix any issues that arise so don't be afraid to ping me. All the best.