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

php-cs-fixer allow risky option #1614

Merged
merged 7 commits into from
Apr 21, 2017
Merged

Conversation

JonidBendo
Copy link
Contributor

@JonidBendo JonidBendo commented Apr 20, 2017

What does this implement/fix? Explain your changes.

Adds an option for allow-risky in php-cs-fixer so you can apply risky rules if you want to.

Does this close any currently open issues?

I am not sure.

Any other comments?

none.

Checklist

Check all those that are applicable and complete.

  • Merged with latest master branch
  • Added examples for testing to examples/ directory
  • Travis CI passes (Mac support)
  • AppVeyor passes (Windows support)

@Glavin001
Copy link
Owner

To confirm, this option is only available on PHP-CS-Fixer v2?

@Glavin001 Glavin001 self-requested a review April 20, 2017 17:18
@Glavin001 Glavin001 self-assigned this Apr 20, 2017
@JonidBendo
Copy link
Contributor Author

@Glavin001 This is actually something that I cannot confirm 100%. It seems to be an option for version 2 only and I only know about it because cs-fixer will return an error message and apply no beatification if:

  • This option is not set to yes
  • Rules that are considered risky are included in the rules to apply

@@ -5852,6 +5852,24 @@
"namespace": "php"
}
},
"allow_risky": {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be automatically generated. I do not see a change to the PHP language adding allow_risky option?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Glavin001 I haven't really worked with modules before, this was my first time and I did not know about the autogeneration part, but i got it now, I also committed the change to the language file.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No problem! Getting closer.

@@ -44,6 +44,12 @@ module.exports = {
type: 'string'
default: ""
description: "Add rule(s). i.e. line_ending,-full_opening_tag,@PSR2 (PHP-CS-Fixer 2 only)"
allow_risky:
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great! Don't forget to regenerate the documentation with npm run docs.

See https://github.com/Glavin001/atom-beautify/blob/master/docs/add-languages-and-beautifiers.md#options-documentation for details.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I should have probably seen this documentation from start 👍 . Well everything should be ready now.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good!

type: 'string'
default: "no"
enum: ["no", "yes"]
description: "allow risky rules to be applied!"
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you add (PHP-CS-Fixer 2 only)? I should have noticed this sooner, sorry about that.

@@ -24,6 +24,7 @@ module.exports = class PHPCSFixer extends Beautifier
"fix"
"--rules=#{options.rules}" if options.rules
"--config=#{configFile}" if configFile
"--allow-risky=#{options.allow_risky}" if options.allow_risky
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So we will always pass in allow_risky?

php-cs-fixer --allow-risky=yes
php-cs-fixer --allow-risky=no

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Glavin001 I thought the if there is not passing this option as long as the allow_risky is not changed (not in the configuration file), after changed yes.

I know for sure that if not defined its set to no by default. So the general behaviour would be.

  1. As long as it is not in the config file it will not be included (I am not sure about this)
  2. If its set to no it will be included but nothing will change from the default behaviour
  3. If it is set to yes then rules like symfony and so on (that are considered risky) may be applied

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds good! I think this is good as is.

As long as it is not in the config file it will not be included (I am not sure about this)

The default value should be present, so no. So as long as php-cs-fixer --allow-risky=no is valid, then we are all good.

@Glavin001
Copy link
Owner

Looking good! I would just like to add a note about (PHP-CS-Fixer 2 Only) because this is not known from the information provided to non-developer users. Thanks!

Copy link
Owner

@Glavin001 Glavin001 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Waiting on Travis CI to pass then will merge!

@Glavin001 Glavin001 merged commit 8ae9669 into Glavin001:master Apr 21, 2017
@Glavin001
Copy link
Owner

Thank you for contributing!

@Glavin001 Glavin001 added this to the v0.30.0 milestone Apr 21, 2017
@Glavin001
Copy link
Owner

Published to v0.29.24

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants