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

[Fix #3904] Build config & sync cop descriptions #6600

Conversation

garettarrowood
Copy link
Contributor

@garettarrowood garettarrowood commented Dec 23, 2018

This PR addresses #3904. It deduplicates Cop descriptions by loading the config and replacing its Descriptions with the ones held in yard. That makes the source code the single source of truth for descriptions. This was heavily inspired by @backus 's implementation in rubocop-rspec. (Thank you sir!)

There are a lot of consequences of loading and dumping YAML. Here are some of the changes you will find in config/default.yml that are noteworthy but (imo) seem ok.

  • Lists are not indented - ref: https://stackoverflow.com/questions/17014460/yaml-indentation-for-array-in-hash
  • single quotes are generally replaced with double quotes. This appears to be when the string contains special characters like * and #. They are not replaced in VersionAdded and VersionChanged settings.
  • Settings such as CacheRootDirectory: ~ are replaced with CacheRootDirectory: . I don't like this, but it's still valid yaml. It seems to boil down to aesthetic, though.
  • Descriptions include line feeds. This doesn't seem like a big deal because those line feeds are following the 80 character Metrics/LineLength setting enforced in the repo already. So comments will appear uniform in the file.

But. There is a deal breaking change here. All comments are stripped from the config. I looked around for a solution and only found ones involving adding comments in one line at a time. While searching for alternative yaml parsers besides YAML/Pysch, I found this blog post which suggests the only comment preserving YAML parser is ruamel, which is for Python.

Possible solutions:

  1. Move the comments into the config. Add keys such as AllowSymlinksInCacheRootDirectoryDescription and give it the associated comment as a value to save.
  2. Don't mix these concerns. Put documentation in something else, designed for it, instead of a configuration file.
  3. Explore trying to make all of these updates using other Ruby libraries such as File, instead of loading the config itself.
  4. Something I haven't thought of. Have an idea?

Last note: I believe it is to our benefit to load config, make a change, and the dump it back out. After syncing descriptions, we could explore syncing any/all other values. Procedurally updating our configuration feels like a step in the right direction.

Thoughts and ideas?


Before submitting the PR make sure the following are checked:

  • Wrote good commit messages.
  • Commit message starts with [Fix #issue-number] (if the related issue exists).
  • Feature branch is up-to-date with master (if not - rebase it).
  • Squashed related commits together.
  • Added tests.
  • Added an entry to the Changelog if the new code introduces user-observable changes. See changelog entry format.
  • The PR relates to only one subject with a clear title
    and description in grammatically correct, complete sentences.
  • Run bundle exec rake default. It executes all tests and RuboCop for itself, and generates the documentation.

Rakefile Outdated Show resolved Hide resolved
lib/rubocop/config_formatter.rb Outdated Show resolved Hide resolved
lib/rubocop/config_formatter.rb Outdated Show resolved Hide resolved
@garettarrowood
Copy link
Contributor Author

@Darhazer - Thanks for the comments! Any opinion on an approach to preserving the config comments? -- I'm leaning towards moving them directly into the config (option 1), but would love a thumbs up from a collaborator/owner before making such an update. -- @bbatsov , what do you think?

@bbatsov
Copy link
Collaborator

bbatsov commented Dec 27, 2018

There are a lot of consequences of loading and dumping YAML. Here are some of the changes you will find in config/default.yml that are noteworthy but (imo) seem ok.

Why do you even load and dump it? I'd just merge the data from the config with whatever we get from the source file without actually regenerating the file itself.

@garettarrowood garettarrowood force-pushed the deduplicate_cop_documentation branch 2 times, most recently from 0ecd7b5 to 98b769a Compare December 29, 2018 15:58
This is here for me to check diffs as I play around with the code. This commit will eventually be squashed.
@bbatsov
Copy link
Collaborator

bbatsov commented Mar 16, 2019

@garettarrowood What's the state here?

@garettarrowood
Copy link
Contributor Author

@bbatsov -
I'm finding it difficult to update the yaml config file using Ruby File classes and not YAML libraries (because they all wipe away the code comments when the config is regenerated). It's possible to find the line in the config file to insert the cop's Description, and then insert it. But I have not yet found how to then detect the formatting/how many lines the outdated description is, and then remove it.

Perhaps there is a technique to go about this that has escaped me so far. I'm open to suggestions :).

@garettarrowood
Copy link
Contributor Author

Perhaps I could cycle through the file twice. The first time removing all the Descriptions. Then cycle through it again and add the ones from the cop files themselves.

It seems inefficient, and a lot of extra file operations than would be necessary. But I can give it a go again with that idea.

@stale
Copy link

stale bot commented May 8, 2019

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contribution and understanding!

@stale stale bot added the stale Issues that haven't been active in a while label May 8, 2019
@stale
Copy link

stale bot commented Jun 7, 2019

This issues been automatically closed due to lack of activity. Feel free to re-open it if you ever come back to it.

@stale stale bot closed this Jun 7, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
stale Issues that haven't been active in a while
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants