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

Add new aa-comma-style all-except-single-line. #80

Merged

Conversation

justinMBullard
Copy link
Contributor

@justinMBullard justinMBullard commented Oct 19, 2022

The all-except-single-line option will enforce "always" for multiple line AA but for single line AA it will enforce "no-dangling".

The all-except-single-line option is here to help with creating more line independent when an AA is extended. For multiple line AA, always is preferred. Always will allow for the addition to AA at the end without requiring a change to the previous line.

However for a single line AA, always just adds an extra , and does not create line independence. For a single line AA it is better to use no-dangling. This will keep the AA smaller.

…ce "always" for multiple line AA but for single line AA it will enforce "no-dangling".
@elsassph
Copy link
Collaborator

Can you show an example where an extra , appears but you think should not?

Maybe it's a bug because single line AA should probably not have a dangling coma even with the dangling rule.

@justinMBullard
Copy link
Contributor Author

@ elsassph I want to be able to support always but not require the last comma for a single line.
The goal is to have a single style that supports both style2 and style4. Style2 for its ability to create more independent diff when changed, Style4 for its brevity.

sub syle2()
    a = {
        a1: 1,
        a2: 2, 'comment
        a3: 3,
    }
end sub

and

sub style4()
    a = { a1: 1, a2: 2, a3: 3 }
end sub

@elsassph
Copy link
Collaborator

style4 shouldn't have a dangling comma IMHO even with the option enabled. Let's just fix that without adding an option :D

@justinMBullard
Copy link
Contributor Author

@elsassph Thanks. I made the change as you suggest.

@justinMBullard
Copy link
Contributor Author

Thank you @elsassph. I think I have made all the required changes.

@elsassph
Copy link
Collaborator

Sorry @justinMBullard some tests seem to be failing...

@justinMBullard
Copy link
Contributor Author

@elsassph Sorry there was a second test that I wasn't aware of that needed updating. I just fixed that. This is my first time working on this project and I am still learning how to navigate everything. Thank you for patients. I think everything should be good now.

@TwitchBronBron TwitchBronBron merged commit 5d8b75b into rokucommunity:master Oct 28, 2022
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.

3 participants