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

Layout/AlignHash allow two options #6649

Merged
merged 4 commits into from May 13, 2019
Merged

Conversation

@stoivo
Copy link
Contributor

stoivo commented Jan 9, 2019

I my company we like to use Hashes in both table and key format. We would like to have rubucop allow both so I make a path an it seams to work for me.
What I want is

# bad
{
  :foo => bar,
   :ba => baz
}

# good
{
  :foo => bar,
  :ba => baz
}
{
  :foo => bar,
  :ba  => baz
}

I wanted to open the PR to check if it would be accepted if I can submitt a nice PR for it. I can add more spec. I wil do so if you think you want this feature

Right now I made it an option not_<any_of_the_setting>. I think it would be nicer if you could spesifi it as a list in .rubocop.yml

Layout/AlignHash:
  EnforcedHashRocketStyle: 
    - table
    - key
  # vs
  EnforcedHashRocketStyle: not_separator

Thoughts? Do you have an example where you do this?
If I can change it to a list the first one can be the format to autofix to.

Edit: after a little more testing is should autofix to the the format how as the least amoint of offences.


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.
@stoivo stoivo force-pushed the stoivo:layout_hash_align branch 2 times, most recently from 751a7a5 to 6473ec8 Jan 23, 2019
@stoivo stoivo changed the title WIP: Layout/AlignHash allow two options Layout/AlignHash allow two options Jan 23, 2019
@stoivo

This comment has been minimized.

Copy link
Contributor Author

stoivo commented Jan 23, 2019

I should add some spec to this.
Other thant that I think it's ready

@stoivo stoivo force-pushed the stoivo:layout_hash_align branch from 6473ec8 to 02e11c1 Jan 25, 2019
@stoivo

This comment has been minimized.

Copy link
Contributor Author

stoivo commented Jan 25, 2019

I have written spec. I would very much like some feedback on this.

@stoivo

This comment has been minimized.

Copy link
Contributor Author

stoivo commented Mar 4, 2019

Hi, @pocke
I see you have merges some other MR here would you review this MR too?

@pocke pocke self-requested a review Mar 5, 2019
@stoivo

This comment has been minimized.

Copy link
Contributor Author

stoivo commented Mar 12, 2019

I will rebase after it is review so I don't wast to much time rebasing and force pushing

@bbatsov

This comment has been minimized.

Copy link
Collaborator

bbatsov commented Mar 17, 2019

Sorry about the slow turnaround on this. I'll take a look a closer look soon. In general I'm wary of adding 3 more styles to this cop, if we probably need just one. From what I gathered most people want something that allows a combination of table or key, right?

@stoivo

This comment has been minimized.

Copy link
Contributor Author

stoivo commented Mar 17, 2019

Thank you for taking a look at it.
Yes, we want a combination of table or key. I think we kan remove the two others, I added those because it was't too much work. We wnt to enforce one style in one hash, either table or key.

@pocke

This comment has been minimized.

Copy link
Member

pocke commented Mar 17, 2019

Sorry for the late reply.
I have the same concern. If a style is added to the cop in the future, no_* styles behaviour is changed.

How about making EnforcedStyle receiving multiple options?
exmaple:

EnforcedStyle:
  - table
  - key

I think it is easy to use, but it is inconsistent with other cops because other cops do not support multiple options.

If you and many users need to allow table and key, I think adding one option is good. The option allows table and key styles.
And I'd like to rename the option to table_and_key. Because no_* option will be changed the behaviour if an option is added to the cop in the future.

@bbatsov

This comment has been minimized.

Copy link
Collaborator

bbatsov commented Mar 18, 2019

I think table_or_key might be a better name, unless we come up with a single word to describe this.

How about making EnforcedStyle receiving multiple options?

That's a pretty interesting idea in general - make it possible to accept several styles where it makes sense. It would probably be reasonable simple to do this, and it might be my preference. @rubocop-hq/rubocop-core What do you think about this idea?

@stoivo

This comment has been minimized.

Copy link
Contributor Author

stoivo commented Mar 18, 2019

I think the idea of supporting either a string or a list would be very cool and flexible. And when you enter all thee formats, any format is valid but only one format in one hash.

I did one attempt at making it a list in the beginning but faced some dificulities. I would be happy diving it a second time, with some mentoring and pointing me where to look.

@bbatsov

This comment has been minimized.

Copy link
Collaborator

bbatsov commented Mar 18, 2019

The only small problem is which style to suggest in case a hash doesn't match any of them. Probably the one that was specified first.

@stoivo

This comment has been minimized.

Copy link
Contributor Author

stoivo commented Mar 19, 2019

@bbatsov, yes to the one with leaset offences. But thats allreade fixed
https://github.com/stoivo/rubocop/blob/master/lib/rubocop/cop/layout/align_hash.rb#L213

EDIT: now it is implemented by the one woth least offences. Since if you add one more element to an element to an existing hash correct as least as possible.

@stoivo

This comment has been minimized.

Copy link
Contributor Author

stoivo commented Mar 19, 2019

I think thats better becuase If you have

{
  ab:    123,
  assdf: 12332,
  asf:   12332,
}

And you add one element

{
  ab:    123,
  assdf: 12332,
  asf:   12332,
  b: true,
}

You probably want to format it to table if you allow both key and table

@bbatsov

This comment has been minimized.

Copy link
Collaborator

bbatsov commented Apr 3, 2019

So, what are doing here? Do we agree to try the multiple enforced styles approach?

@stoivo

This comment has been minimized.

Copy link
Contributor Author

stoivo commented Apr 3, 2019

I can use the name table_or_key instead of not_separator.
@bbatsov What do you think of autocorrection to the one with least offences?

CHANGELOG.md Outdated Show resolved Hide resolved
@bbatsov

This comment has been minimized.

Copy link
Collaborator

bbatsov commented Apr 4, 2019

I actually prefer @pocke's idea and autocorrecting using whatever is first in the EnforcedStyle list. I don't like relying on offenses, because that's non deterministic.

@stoivo stoivo force-pushed the stoivo:layout_hash_align branch from 7125db2 to e5dc67c Apr 4, 2019
@stoivo

This comment has been minimized.

Copy link
Contributor Author

stoivo commented Apr 4, 2019

I actually prefer @pocke's idea and autocorrecting using whatever is first in the EnforcedStyle list. I don't like relying on offenses, because that's non deterministic.

Did you se my comment about what happens if you add something to an existing hash but without 100% match?

I think thats better becuase If you have

{
  ab:    123,
  assdf: 12332,
  asf:   12332,
}

And you add one element

{
  ab:    123,
  assdf: 12332,
  asf:   12332,
  b: true,
}

You probably want to format it to table if you allow both key and table

I just want to say, you have more experence maintaining rubocop so I will do just want to say :)
I pushed a commit where I merged the commits and rebased away the merge commits.
It is't working 100% and I dont have time to fix it today. Will do it shortly.

@bbatsov

This comment has been minimized.

Copy link
Collaborator

bbatsov commented Apr 4, 2019

I just want to say, you have more experence maintaining rubocop so I will do just want to say :)
I pushed a commit where I merged the commits and rebased away the merge commits.
It is't working 100% and I dont have time to fix it today. Will do it shortly.

Yeah, I did. I don't consider this a problem as in your editor you'd know what to do - you see some warning, but you also know it will disappear once you change it. The auto-correct should just pick the first option:

EnforcedStyle:
  - table
  - key

In this case - table. Just to be clear - I strongly prefer to have a list of options for EnforcedStyle than a dedicated table_or_key style (although I can live with it). I this case I'd default to key i guess.

@bbatsov

This comment has been minimized.

Copy link
Collaborator

bbatsov commented Apr 18, 2019

@stoivo Ping :-)

@stoivo stoivo force-pushed the stoivo:layout_hash_align branch 4 times, most recently from 2ae82cb to e0212f4 Apr 26, 2019
@stoivo stoivo force-pushed the stoivo:layout_hash_align branch from e0212f4 to 0306ae9 Apr 26, 2019
@stoivo

This comment has been minimized.

Copy link
Contributor Author

stoivo commented Apr 26, 2019

@bbatsov, thanks for beein patient with me.

Could to review it now?

I am facing a minor issue with rubocop

$ rubocop                                                                                                                                                    15:09:20
Inspecting 1172 files
..................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................C.................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................

Offenses:

lib/rubocop/config.rb:528:5: C: Metrics/AbcSize: Assignment Branch Condition size for validate_enforced_styles is too high. [20.05/17]
    def validate_enforced_styles(valid_cop_names) ...
    ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
lib/rubocop/config.rb:528:5: C: Metrics/MethodLength: Method has too many lines. [16/14]
    def validate_enforced_styles(valid_cop_names) ...
    ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

1172 files inspected, 2 offenses detected

https://github.com/stoivo/rubocop/blob/0306ae96739b4de743ddb3935adc6611e1134b31/lib/rubocop/config.rb#L528-L548

    def validate_enforced_styles(valid_cop_names)
      valid_cop_names.each do |name|
        styles = self[name].select { |key, _| key.start_with?('Enforced') }

        styles.each do |style_name, style|
          supported_key = RuboCop::Cop::Util.to_supported_styles(style_name)
          valid = ConfigLoader.default_configuration[name][supported_key]

          next unless valid
          next if valid.include?(style)
          next if ConfigLoader.default_configuration[name]['AllowListStyles'] &&
                  style.is_a?(Array) &&
                  style.all? { |a_style| valid.include?(a_style) }

          msg = "invalid #{style_name} '#{style}' for #{name} found in " \
            "#{smart_loaded_path}\n" \
            "Valid choices are: #{valid.join(', ')}"
          raise ValidationError, msg
        end
      end
    end

How do you want me to solve this?

@jonas054

This comment has been minimized.

Copy link
Collaborator

jonas054 commented Apr 30, 2019

How do you want me to solve this?

I think you should extract either the contents of the inner each loop, or just the complex expression

ConfigLoader.default_configuration[name]['AllowListStyles'] &&
                  style.is_a?(Array) &&
                  style.all? { |a_style| valid.include?(a_style) }

into a new method. Come up with a name for the new method and you're good to go. 🙂

@stoivo stoivo force-pushed the stoivo:layout_hash_align branch from 0306ae9 to 342cce4 May 1, 2019
CHANGELOG.md Outdated Show resolved Hide resolved
config/default.yml Outdated Show resolved Hide resolved
@@ -174,6 +174,37 @@
end
end

context 'when the configuration includes multiple valid EnforcedStyle' do

This comment has been minimized.

Copy link
@bbatsov

bbatsov May 9, 2019

Collaborator

Should we raise an error if the cop doesn't support multiple style explicitly?

@bbatsov

This comment has been minimized.

Copy link
Collaborator

bbatsov commented May 9, 2019

@stoivo Added a bit of small remarks. Generally the code looks pretty good and we're very close to finally merging this.

@bquorning

This comment has been minimized.

Copy link
Member

bquorning commented May 9, 2019

Should we consider reverting 807b362 as part of this PR?

@bquorning

This comment has been minimized.

Copy link
Member

bquorning commented May 9, 2019

This will fix #6410, right?

@bbatsov

This comment has been minimized.

Copy link
Collaborator

bbatsov commented May 9, 2019

Should we consider reverting 807b362 as part of this PR?

Yes.

stoivo added 2 commits Apr 4, 2019
You can specify table_or_key if you want to allow both table and key
When multiple hash formats is accept the hash need to be either one or the other.
@stoivo stoivo force-pushed the stoivo:layout_hash_align branch from 342cce4 to efc8dc2 May 13, 2019
You can specify table or key or separator or a combination of any of these. If
you allow both table and key any hash might be either.
@stoivo stoivo force-pushed the stoivo:layout_hash_align branch from efc8dc2 to bca03f9 May 13, 2019
This reverts commit da8c541.
@stoivo

This comment has been minimized.

Copy link
Contributor Author

stoivo commented May 13, 2019

Ready for an onther review.

I reverted the commit and did some small modifications to resolve conflicts.

@bbatsov bbatsov merged commit 971769a into rubocop-hq:master May 13, 2019
22 checks passed
22 checks passed
ci/circleci: cc-setup Your tests passed on CircleCI!
Details
ci/circleci: cc-upload-coverage Your tests passed on CircleCI!
Details
ci/circleci: documentation-checks Your tests passed on CircleCI!
Details
ci/circleci: jruby-9.2-ascii_spec Your tests passed on CircleCI!
Details
ci/circleci: jruby-9.2-rubocop Your tests passed on CircleCI!
Details
ci/circleci: jruby-9.2-spec Your tests passed on CircleCI!
Details
ci/circleci: ruby-2.3-ascii_spec Your tests passed on CircleCI!
Details
ci/circleci: ruby-2.3-rubocop Your tests passed on CircleCI!
Details
ci/circleci: ruby-2.3-spec Your tests passed on CircleCI!
Details
ci/circleci: ruby-2.4-ascii_spec Your tests passed on CircleCI!
Details
ci/circleci: ruby-2.4-rubocop Your tests passed on CircleCI!
Details
ci/circleci: ruby-2.4-spec Your tests passed on CircleCI!
Details
ci/circleci: ruby-2.5-ascii_spec Your tests passed on CircleCI!
Details
ci/circleci: ruby-2.5-rubocop Your tests passed on CircleCI!
Details
ci/circleci: ruby-2.5-spec Your tests passed on CircleCI!
Details
ci/circleci: ruby-2.6-ascii_spec Your tests passed on CircleCI!
Details
ci/circleci: ruby-2.6-rubocop Your tests passed on CircleCI!
Details
ci/circleci: ruby-2.6-spec Your tests passed on CircleCI!
Details
ci/circleci: ruby-head-ascii_spec Your tests passed on CircleCI!
Details
ci/circleci: ruby-head-rubocop Your tests passed on CircleCI!
Details
ci/circleci: ruby-head-spec Your tests passed on CircleCI!
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
@bbatsov

This comment has been minimized.

Copy link
Collaborator

bbatsov commented May 13, 2019

Good job! There are some small things that'd I'd like us to polish down the road and we have to document somewhere the ability to have cops that support multiple styles, but I'll merge the PR in its current state as it's pretty big and you've been wasting a lot of efforts keeping it up to date. Thanks! 🙇

@stoivo

This comment has been minimized.

Copy link
Contributor Author

stoivo commented May 13, 2019

Thank you too. I wm happy to help out a more but I think is is better if you do the polish and add the documentation where it belongs. Very happy with submitting a patch to rubocop. Love the tool ❤️

koic added a commit to koic/rubocop that referenced this pull request May 21, 2019
Follow up rubocop-hq@8a7ee84.

This PR fixes the following CI errors.

```console
bundle exec rake

  1) RuboCop Project changelog entry after version 0.14.0 has a link to
  the contributors at the end
     Failure/Error: expect(entries).to all(match(/\(\[@\S+\]\[\](?:,
  \[@\S+\]\[\])*\)$/))

       expected ["*
       [rubocop-hq#6649](rubocop-hq#6649):
       `Layout/AlignHash` supports list of opt...552): `RaiseArgs`
       allows exception constructor calls with more than one 1
       argument. ([@bbatsov][])"] to all match /\(\[@\S+\]\[\](?:,
       \[@\S+\]\[\])*\)$/

          object at index 3 failed to match:
             expected "*
             [rubocop-hq#7052](rubocop-hq#7052):
             Add `AllowComments` option to `
             Lint/HandleExceptions`. ([@tejasbubane][]) " to match
             /\(\[@\S+\]\[\](?:, \[@\S+\]\[\])*\)$/
     # ./spec/project_spec.rb:128:in `block (5 levels) in <top (required)>'

(snip)

  2) RuboCop Project changelog entry body ends with a punctuation
     Failure/Error: expect(bodies).to all(match(/[\.\!]$/))

       expected ["`` supports list of options.", "Add `` config option
       to ``.", "Add `` to ``.", "Add `` option to ``...nclosed in
       braces are not noticed.", "Received malformed format string
       ArgumentError from rubocop."] to all match /[\.\!]$/

          object at index 3 failed to match:
             expected "Add `` option to ``. ([@tejasbubane][]) " to
             match /[\.\!]$/
     # ./spec/project_spec.rb:187:in `block (5 levels) in <top (required)>'
```

https://circleci.com/gh/rubocop-hq/rubocop/55378
koic added a commit to koic/rubocop that referenced this pull request May 21, 2019
Follow up rubocop-hq@8a7ee84.

This PR fixes the following CI errors.

```console
bundle exec rake

(snip)

  1) RuboCop Project changelog entry after version 0.14.0 has a link to
  the contributors at the end
     Failure/Error: expect(entries).to all(match(/\(\[@\S+\]\[\](?:,
  \[@\S+\]\[\])*\)$/))

       expected ["*
       [rubocop-hq#6649](rubocop-hq#6649):
       `Layout/AlignHash` supports list of opt...552): `RaiseArgs`
       allows exception constructor calls with more than one 1
       argument. ([@bbatsov][])"] to all match /\(\[@\S+\]\[\](?:,
       \[@\S+\]\[\])*\)$/

          object at index 3 failed to match:
             expected "*
             [rubocop-hq#7052](rubocop-hq#7052):
             Add `AllowComments` option to `
             Lint/HandleExceptions`. ([@tejasbubane][]) " to match
             /\(\[@\S+\]\[\](?:, \[@\S+\]\[\])*\)$/
     # ./spec/project_spec.rb:128:in `block (5 levels) in <top (required)>'

(snip)

  2) RuboCop Project changelog entry body ends with a punctuation
     Failure/Error: expect(bodies).to all(match(/[\.\!]$/))

       expected ["`` supports list of options.", "Add `` config option
       to ``.", "Add `` to ``.", "Add `` option to ``...nclosed in
       braces are not noticed.", "Received malformed format string
       ArgumentError from rubocop."] to all match /[\.\!]$/

          object at index 3 failed to match:
             expected "Add `` option to ``. ([@tejasbubane][]) " to
             match /[\.\!]$/
     # ./spec/project_spec.rb:187:in `block (5 levels) in <top (required)>'
```

https://circleci.com/gh/rubocop-hq/rubocop/55378
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.