Skip to content

Added vendor/project namespace, restructured folders for PSR-4 #58

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

Merged
merged 6 commits into from
Jul 29, 2016

Conversation

pbrisbin
Copy link
Contributor

@pbrisbin pbrisbin commented Jul 29, 2016

This includes #55 and is where I plan to configure CC platform and make any
other fixes before moving on that track.

/cc @hollodotme

  • Add CC
  • Address CC issues
  • Fix bug that broke test reporting

hollodotme and others added 2 commits July 29, 2016 12:13
@pbrisbin
Copy link
Contributor Author

@hollodotme I tried to enable the phpcodesniffer engine on this repo and encountered a slew of issues for tabs that should be spaces, which it looks like was a change you made. Do you strongly prefer tabs / is there something about PSR-4 that requires it? If not, I'm going to go ahead and put them all back to spaces (and clean up some other formatting issues) so I can enable phpcodesniffer.

@hollodotme
Copy link
Contributor

@pbrisbin No, there is generally no requirement for tabs in the PSR standards or elsewhere. I prefer tabs because they have no semantic meaning, while spaces do. Btw, afaik it is a setting in the codesniffer if it should check for tabs or spaces. As mentioned, my PhpStorm default formatting rules apply tabs when formatting code. Anyway, it's your code, you choose. ;)

@pbrisbin
Copy link
Contributor Author

Thank you. I've changed three style things as identified by phpcodesniffer:

  • (4) spaces, not tabs
  • (arg), not ( arg )
  • if {, not if\n{ (same for for and switch)

In all cases we pefer it this way, and it happens to be phpcodesniffer's defaults, so that's nice. I apologize if this makes rebasing your other branches more difficult.

@hollodotme
Copy link
Contributor

Really, no problem.

enabled: true
config:
languages:
- ruby
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit, but can we clean this file up a bit? I don't think we need all the duplication language, nor do we need ratings paths for *.js, etc.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Strange. I didn't even look after I initd. I wonder why this was added.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think all of those extensions get added because they're all potentially analyzeable by the duplication engine. We should make that smarter ;)

Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder why this was added.

Because our init code isn't that smart 😀. We see some regexes for some languages duplication supports, so we enable the engine with a default config, but don't tailor that config to which regexes matched.

@wfleming
Copy link
Contributor

LGTM

* @see \Symfony\Component\Console\Application::getDefaultCommands()
*/
protected function getDefaultCommands()
{
// Keep the core default commands to have the HelpCommand
// which is used when using the --help option
$defaultCommands = parent::getDefaultCommands();
$defaultCommands = parent::getDefaultCommands();
Copy link
Contributor

Choose a reason for hiding this comment

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

extra space here?

Copy link
Contributor

Choose a reason for hiding this comment

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

It looks like aligned tokens are actually the expected style here.

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.

4 participants