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

Simplify and allow to configure rubocop formatter #403

Merged
merged 5 commits into from Jan 28, 2021

Conversation

jimeh
Copy link
Contributor

@jimeh jimeh commented Jan 6, 2021

This effectively simplifies and enhances how Solargraph interacts with RuboCop in three specific ways. Below is a summary, more details about each change is in the commit messages.

  • Removes the need of creating/reading/writing any temporary files on disk when formatting code, by passing it in to rubocop in-memory via its STDIN options.
  • The STDIN method means that the real original filepath on disk can be provided to rubocop, allowing it to correctly find any relevant .rubocop.yml config files itself, negating the need for Solargraph to have it's own implementation of rubocop config file finding. So no -c option is needed within the formatter, nor was it needed within the diagnostics implementation as that already used the STDIN method.
  • A new formatter option is available within Solargraph's own configuration file, which allows some basic customization of rubocop. This is useful if you want to disable certain cops when formatting from within your editor, but you otherwise want the cops enabled when manually running rubocop.

Update: I'm not sure why it's not displayed here in the PR, but the Travis-CI build has passed: https://travis-ci.org/github/castwide/solargraph/builds/753192754


I believe this PR will fix #401, as no temp directory/file is used anymore, and rubocop's config is correctly picked up by rubocop itself without any intervention by Solargraph.

@jimeh
Copy link
Contributor Author

jimeh commented Jan 6, 2021

(Updated to reflect latest configuration options in this PR)

For reference, I personally use the following configuration for Solargraph:

~/.config/solargraph/config.yml

---
exclude:
  - vendor/**/*
  - ".bundle/**/*"
reporters:
  - rubocop
  - require_not_found
  - typecheck
max_files: 5000
formatter:
  rubocop:
    cops: all
    except:
      - Lint/Debugger
      - Lint/UnusedBlockArgument
      - Lint/UnusedMethodArgument
      - Style/EmptyMethod

For a short explanation of why I disable each of those cops, see the Disabled Cops section in the README of my rubocopfmt.el Emacs package.

@castwide
Copy link
Owner

Thanks, @jimeh. I'm testing the PR locally and everything seems to work.

The one issue is the formatter option. An upcoming feature will allow users to change formatting tools via plugins, with the default configuration being formatter: rubocop. Maybe we could use a rubocop option instead?

@jimeh
Copy link
Contributor Author

jimeh commented Jan 10, 2021

@castwide I'm happy to change the config options. Personally I think having formatter involved somehow might help make it clearer that these options only applies to the rubocop based formatter, and not the linter/diagnostics.

To make things simple, pick from A, B, and C below, or feel free to suggest a different structure that you prefer :)

A:

rubocop:
  cops: all
  extra_args:
    - --except
    - Lint/Debugger,Lint/UnusedBlockArgument,Lint/UnusedMethodArgument,Style/EmptyMethod

B:

formatter:
  rubocop:
    cops: all
    extra_args:
      - --except
      - Lint/Debugger,Lint/UnusedBlockArgument,Lint/UnusedMethodArgument,Style/EmptyMethod

C:

rubocop:
  formatter:
    cops: all
    extra_args:
      - --except
      - Lint/Debugger,Lint/UnusedBlockArgument,Lint/UnusedMethodArgument,Style/EmptyMethod

While we're on the topic of config, I'm planning to add except and only as legit options later today, so I can specify them directly as YAML arrays, rather than through extra_args.

@castwide
Copy link
Owner

I think either B or C would be best. I slightly prefer B, but the implementation might be a little hairy, since the formatter option would need to be capable of recognizing the formatter name by either a string (formatter = "rubocop") or a hash key (formatter = {"rubocop" => {"cops" => "all"}}).

@jimeh
Copy link
Contributor Author

jimeh commented Jan 14, 2021

@castwide I've gone with option B as it was your preferred variant, and honestly it was my preferred one too.

It did lead to slightly annoying checking logic with the new config_for method, that ensures that the config variable is always as Hash, so things don't break regardless of what might or might not be in the solargraph config file.

I also added the except and only config options I mentioned, so the config now covers all of my own usecases at least.

@castwide
Copy link
Owner

@jimeh Thanks for making these changes. With 0.40.2 released today, I expect to merge this PR shortly and release it in 0.41.0.

@jimeh
Copy link
Contributor Author

jimeh commented Jan 18, 2021

@castwide let me know if you want me to rebase on top of master or something before you merge :)

@jsmestad
Copy link

Waiting patiently for this to drop so I can start using solargraph again 🤞 🎉

@pelletencate
Copy link

pelletencate commented Jan 27, 2021

@jsmestad Anything stopping you from temporarily using @jimeh's branch? Works wonders for me!

# Gemfile

  # TODO: Revert on resolution of castwide/solargraph#403
  # gem "solargraph", "~> 0.40.0"
  gem "solargraph", github: "jimeh/solargraph", branch: "fix-rubocop-formatting"

By supplying the file content to RubyCop as a in-memory string the same
way as RuboCop's own CLI tool handles STDIN, we completely avoid having
to write any temporary files to disk, and formatting can occur purely
in-memory.

This also has the benefit of Solargraph no longer needing to be
responsible for finding and passing along any config file paths. RuboCop
will correctly find any relevant .rubocop.yml files for the file being
formatted, as RuboCop behaves exactly as if it was running against the
real file in it's original location on disk, except with the content it
is formatting coming from the in-memory string rather than reading it
from the file on disk.

A new blank formatter is also passed to RuboCop, just to make it print
as little as possible to STDOUT, since we don't need and ignore the
STDOUT produced.
Allows customizing if format command should run safe, or all cops when
formatting files.

Also includes a `extra_args` option to pass custom CLI arguments to
RuboCop. If you primarily only format via solargraph interactively from
an editor, it might be useful to disable certain cops that causes
annoyances in interactive use. For example:

    formatter:
      cops: safe # use "all" to include unsafe cops
      extra_args:
        - --except
        - Lint/Debugger,Lint/UnusedBlockArgument,Lint/UnusedMethodArgument,Style/EmptyMethod
RuboCop will itself find and use any relevant configuration files based
on the file path of the file it is analyzing. Hence there is no need for
Solargraph to have it's own implementation of this logic.
Solargraph will gain support for multiple formatters soon, so let's
ensure configuration for the rubocop formatter is future proof.

New nested configuration:

    formatter:
      rubocop:
        cops: all
        extra_args: []

Previously:

    formatter:
      cops: all
      extra_args: []
They take a list or comma separated string of cops, which will be passed
to rubocop as --except and/or --only.

The "except" option is useful to disable cops which can cause annoyance
when using solargraph's formatter as a before save hook in text
editors.

I'm less aware of what the "only" option might be useful for, but might
as well add it so both are covered.
@jimeh
Copy link
Contributor Author

jimeh commented Jan 28, 2021

I've just rebased my changes here on top of latest changes in master.

@castwide
Copy link
Owner

Thanks, @jimeh. I'll go ahead and merge this now. I was on the fence about whether to publish this without the upcoming formatter plugin support, but it should be fine either way.

@castwide castwide merged commit d9d2abf into castwide:master Jan 28, 2021
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.

Temp directory formatting breaks rubocop configuration
4 participants