Skip to content
This repository was archived by the owner on Sep 22, 2019. It is now read-only.

Resolves issue #11 #12

Closed
wants to merge 1 commit into from

Conversation

peter279k
Copy link
Contributor

@peter279k peter279k commented Aug 31, 2019

Changed log

  • Resolves issue #11.
  • Integrating PHP-CS-Fixer and do PSR-1 and PSR-2 coding style checks defined in .cs.php config.
  • Before cloning the YOURLS repo, using php-cs-fixer to do coding style check.
    This can avoid checking the coding style on YOURLS repository.
  • It should define PHPUnit and PHP-CS-Fixer versions inside require-dev in composer.json because they're in development environment.

@LeoColomb
Copy link
Member

Thanks a lot for this PR @peter279k!

Some notes:

  • Integrating PHP-CS-Fixer

I do prefer phpcs. It behave like a linter but does not try to fix files by default, which is more common and "safer" for development.

  • do PSR-1 and PSR-2 coding style

👎 for now. As I mentioned in #11, YOURLS is currently not compliant with these standards, they can't be enforce like this.

  • Before cloning the YOURLS repo, using php-cs-fixer to do coding style check.
    This can avoid checking the coding style on YOURLS repository.

I don't follow you. What is avoided?

  • they're in development environment.

Not in this repository. We are talking about the unit tests repo, which directly require PHPUnit.
Anyway, this is unrelated with the coding style.


For all these reasons, I'm rejecting this PR, sorry.

@LeoColomb LeoColomb closed this Sep 3, 2019
@peter279k
Copy link
Contributor Author

@LeoColomb, thanks for your reply. And how to do this coding style check?

  • Perhaps we can define the basic coding style to check. Such as spaces, variable naming and so on.
  • I misunderstand, and I know about the PHP_CodeSniffer.

If possible, could you reopen this PR? And we can figure out basic coding style and create the phpcs.xml to be consistency for this repository.

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

Successfully merging this pull request may close these issues.

2 participants