Fix generating ignore list when one exists already

[craigw]

Hello! thanks for all your effort creating and maintaining this codebase.

I've run into an issue while getting started using standardrb, and included here a proposed fix.

What's the issue?

Generating an ignore list is done by running Rubocop over all the files in the project to detect which have errors, and then writing that list of files to .standard_todo.yml.

When a file was already ignored in .standard_todo.yml, Rubocop would be configured to not run cops against it. In that case, no errors were reported, so the file would not be written to .standard_todo.yml.

This leads to a situation where if a file is ignored, running standardrb --generate-todo would remove it from the ignore list, and add all other files with errors. Running the same command again would then flip the ignore list.

What's the fix?

By setting the list of ignored files to an empty array before running Rubocop in the genignore runner we can cause Rubocop to list out all files with errors, which then correctly get included in the .standard_todo.yml.

However!

There is a possible issue with this approach: if ignore_defaults is set to true, it's likely that the previously implicitly ignored files will be added to the explicit ignore: list in .standard_todo.yml.

I'm only just starting to get familiar with the codebase, so I can't see an obvious way to ensure that these defaults are not added to the ignore list, and are instead left implicit. If you have some suggestions in this area I'd be happy to dig into it some more.

[searls]

cc/ @mrbiggred who's graciously maintaining this feature!

[mrbiggred]

@craigw thanks for bringing up this issue. I'll take a look at the issue and PR later this week.

[mrbiggred]

@craigw I was able to duplicate the issue you described by doing the below steps. I assume they are similar to what you experienced. Thank you for finding this issue. I'll review your fix as soon as I can. For now it is easy to workaround by just regenerating the todo file with an empty .standard_todo.yml.

Steps to Duplicate

1. Run standardrb --generate-todo on a project with known issues and no existing .standard_todo.yml file. It will generate the expected todo file with the known errors.

2. Run standardrb --generate-todo on the same project. It will re-generate an empty todo file.

Workaround

To workaround this issue just run the --generate-todo a third time with the empty todo file. Another workaround is to delete the .standard_todo.yml and run standardrb --generate-todo.

[mrbiggred]

@craigw thank you for patience. I took a look at your fix and refreshed my memory of how the code loads the .standard.yml and .standard_todo.yml files.

Both the .standard.yml and .standard_todo.yml have an ignore section. The ignore section in the .standard.yml are files that should always be ignored where the ones in the .standard_todo.yml should only be temporarily ignored.

The ignore section of the files are combined in the LoadsYamConfig file:

def construct_config(yaml_path, standard_yaml, todo_path, todo_yaml)
  {
    ruby_version: Gem::Version.new((standard_yaml["ruby_version"] || RUBY_VERSION)),
    fix: !!standard_yaml["fix"],
    format: standard_yaml["format"],
    parallel: !!standard_yaml["parallel"],
    ignore: expand_ignore_config(standard_yaml["ignore"]) + expand_ignore_config(todo_yaml["ignore"]),
    default_ignores: standard_yaml.key?("default_ignores") ? !!standard_yaml["default_ignores"] : true,
    config_root: yaml_path ? Pathname.new(yaml_path).dirname.to_s : nil,
    todo_file: todo_path,
    todo_ignore_files: todo_yaml["ignore"] || []
  }
end

I think a problem with the below fix is it will ignore the files listed in the .standard.yml when it shouldn't.

# lib/standard/runners/genignore.rb

def remove_project_files_from_ignore_list(config)
  options_config = config.rubocop_config_store.instance_variable_get("@options_config")
  options_config["AllCops"] ||= []
  options_config["AllCops"]["Exclude"] = []
end

The best fix I can think of is to not load the .standard_todo.yml file if the --generate-todo option is passed in. The problem is command line arguments are not parsed until both yml files are loaded in the MergesSettings class.

# lib/standard/builds_config.rb

def call(argv, search_path = Dir.pwd)
  standard_yaml_path = determine_yaml_file(argv, search_path, "--config", ".standard.yml")
  todo_yaml_path = determine_yaml_file(argv, search_path, "--todo", ".standard_todo.yml")
  standard_config = @loads_yaml_config.call(standard_yaml_path, todo_yaml_path)

  settings = @merges_settings.call(argv, standard_config)
  Config.new(
	settings.runner,
	settings.paths,
	settings.options,
	@creates_config_store.call(standard_config)
  )
end

I noticed this issue when I was first adding the Todo file but was, and I'm still, not sure the best way to load the command line options before loading the config files.

@craigw I know changing how the command lines are loaded is more then you signed up for but is it something you want to attempt?

@searls do you have any issues with us making changes to the command line? Do you have a better solution then mine?

[searls]

Why not just delete any existing .standard_todo.yml file at the very start of a --generate-todo CLI run?

[craigw]

I'd be happy to have an explore in either direction.

I believe however that configuration is built and .standard_todo.yml is loaded before we know it's a --generate-todo run. In that case it's likely that both approaches (not loading .standard_todo.yml vs deleting the existing .standard_todo.yml) will require very similar changes to the code.

[searls]

Ah I see. Yeah, at this point a minimally invasive approach to ensure the TODO ignore yaml is not merged in when --generate-todo is flagged seems to be necessary

[mrbiggred]

@craigw and @searls sorry but I will be busy with client work until the end of June. It's a project that was delayed in April and May due to Covid but still needs to get done this summer. A good problem to have when so many people are searching for work.

Unfortunately it means I won't be able to work on this issue until July. @craigw do you feel up for updating how the command line (CL) options are parsed? I would do my best to review your CL changes and I'm sure @searls would also be happy to give feedback.

If so feel free to continue working on this PR or close this one and create a new one if need be. I'm not sure we want the CL changes combined with fixing this bug or as two separate PRs. Thoughts?

[searls]

Could someone re-test this since other recent changes to the todo file behavior and let us know if they still think it's necessary?

[SampsonCrowley]

Could this come with an option to disable the todo list during runs? make it possible to run the linter on a single legacy file without deleting the whole list just to check out its current status?