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

[Implement sniff] Have either UNIX or DOS line endings for PHP, JS and CSS files #3

Closed
1 of 2 tasks
jrfnl opened this issue Jul 10, 2016 · 6 comments
Closed
1 of 2 tasks

Comments

@jrfnl
Copy link

jrfnl commented Jul 10, 2016

Rule:

Have either UNIX or DOS line endings, not both. This rule gets applied to PHP, CSS, JS and TXT files.

Ref: https://make.wordpress.org/themes/handbook/review/required/theme-check-plugin/#line-endings

Theme check file covering this rule:

https://github.com/Otto42/theme-check/blob/master/checks/lineendings.php

Decision needed by Theme Review Board:

Currently the Theme Check plugin checks whether both \r as well as \n line endings or combinations thereof are found and throws a warning if that's the case as this can cause a problem with SVN repositories.
The warning leaves it open to the Theme Author to choose whether to use UNIX or DOS line endings as long as they do so consistently.

The existing PHPCS sniff for this - Generic.Files.LineEndings - generates an error.
Additionally the sniff is aimed at one preferred line ending not an either/or type of situation.

The WPCS standard is to throw an error and to require UNIX style \n line endings.

Advice: Follow the WPCS standard and adjust the rule to consistently require UNIX style \n line endings.

To do:

  • If agreed that this should be an error and that UNIX line endings should be required - add existing Generic.Files.LineEndings sniff to the ruleset with the appropriate properties set.
  • Add the rule in the Theme Review handbook to the Requirements page.
@grappler
Copy link
Member

It may be a WARNING in the theme check but the check fails so it should be REQUIRED.

We want to reduce the number of manual requirements with automated. Once it is automated it the theme sniffs should suffice as documention.

@jrfnl
Copy link
Author

jrfnl commented Jul 10, 2016

Once it is automated it the theme sniffs should suffice as documention.

I'm not sure I agree with that.

Especially new theme authors - but probably also existing ones - will probably be more comfortable with reading a list of requirements and recommendations.
Having a list with the rules in the handbook makes it so people are not surprised by (new) rules once they get into the review process.
This is similar to how WPCS handles this. The handbook is leading and only rules which are explicit in the handbook get added to the sniff rulesets.

@grappler
Copy link
Member

Green light to go ahead :
https://make.wordpress.org/themes/2016/07/12/meeting-notes-for-2016-july-12/

Will look for a good way to document the sniffs. Will raise the question at the next team meeting? Perhaps we could track them in a wiki till it is complete.

@grappler
Copy link
Member

The PR has been merged.

@grappler
Copy link
Member

Sorry, still need to update the documentation

@grappler grappler reopened this Jul 16, 2016
@grappler
Copy link
Member

grappler commented Oct 2, 2016

I know the PR has been merged but I think we need to revert this. There is no need for us to require themes to only use of type of line endings.

PHPCS has an internal Warning Internal.LineEndings.Mixed that we can use to make sure the files only have one type of line endings.

The code looks simple enough

        // Check for mixed line endings as these can cause tokenizer errors and we
        // should let the user know that the results they get may be incorrect.
        // This is done by removing all backslashes, removing the newline char we
        // detected, then converting newlines chars into text. If any backslashes
        // are left at the end, we have additional newline chars in use.
        $contents = str_replace('\\', '', $contents);
        $contents = str_replace($this->eolChar, '', $contents);
        $contents = str_replace("\n", '\n', $contents);
        $contents = str_replace("\r", '\r', $contents);
        if (strpos($contents, '\\') !== false) {
            $error = 'File has mixed line endings; this may cause incorrect results';
            $this->addWarning($error, 0, 'Internal.LineEndings.Mixed');
        }

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

No branches or pull requests

2 participants