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

Feature/robo coding standards #6186

Merged

Conversation

Dillon-Brown
Copy link
Contributor

Description

Adds a robo task for automatically fixing coding standard issues and a dist file for PhpCsFixer configurations.

How To Test This

  1. Run composer update
  2. Run ./vendor/bin/robo configure:phpCSFixer
  3. Run ./vendor/bin/robo php:CSFixer

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Final checklist

  • My code follows the code style of this project found here.
  • My change requires a change to the documentation.
  • I have read the How to Contribute guidelines.

Copy link
Contributor

@gymad gymad left a comment

Choose a reason for hiding this comment

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

I like it 👍

@pgorod
Copy link
Contributor

pgorod commented Jul 20, 2018

Cool. Could this tool take care of updating the license header at the top of each file?

@codecov-io
Copy link

codecov-io commented Jul 20, 2018

Codecov Report

Merging #6186 into hotfix will decrease coverage by <.01%.
The diff coverage is 0%.

@@             Coverage Diff              @@
##             hotfix    #6186      +/-   ##
============================================
- Coverage     10.78%   10.78%   -0.01%     
- Complexity    42070    42076       +6     
============================================
  Files          3366     3367       +1     
  Lines        244586   244611      +25     
============================================
+ Hits          26375    26376       +1     
- Misses       218211   218235      +24

@Dillon-Brown
Copy link
Contributor Author

@pgorod That's definitely something i'll look at before finishing with this PR. We can do it through .php_cs.dist but I don't think that's the best approach; for the language files it makes sense to update them all at once but for core files there's no real point in updating them unless the file itself is changed. The php_cs can't really be customised in the way we would need to make it work well with Suite so i'll probably end up creating a more specific robotask.

@Dillon-Brown Dillon-Brown changed the base branch from hotfix to develop December 27, 2018 13:33
@Dillon-Brown Dillon-Brown changed the title *WIP Feature/robo coding standards Feature/robo coding standards Dec 27, 2018
@isleshocky77
Copy link
Contributor

isleshocky77 commented Jan 25, 2019

@Dillon-Brown

@pgorod That's definitely something i'll look at before finishing with this PR. We can do it through .php_cs.dist but I don't think that's the best approach; for the language files it makes sense to update them all at once but for core files there's no real point in updating them unless the file itself is changed. The php_cs can't really be customised in the way we would need to make it work well with Suite so i'll probably end up creating a more specific robotask.

I'm confused by "It can't work that way". Why not add the .dist file and only add the modules/files ready to cleaned up and kept clean? I know you working on cleaning up module by module. So you could just add those modules to the .dist file.

@connorshea
Copy link
Contributor

This needs a rebase, also shouldn't we have travis fail on failed lints?

@Dillon-Brown Dillon-Brown changed the base branch from develop to hotfix February 7, 2019 11:58
@jack7anderson7
Copy link
Contributor

Assessed 👍

@samus-aran samus-aran merged commit aff69c3 into salesagility:hotfix Feb 7, 2019
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.

None yet

9 participants