humanmade / coding-standards Goto Github PK
View Code? Open in Web Editor NEWHuman Made coding standards for modern code
Home Page: https://engineering.hmn.md/how-we-work/style/
Human Made coding standards for modern code
Home Page: https://engineering.hmn.md/how-we-work/style/
We have installed the composer package in a WP plugin, this plugin has a package.json
file with a dependency of react-scripts
- v2.0.4.
The composer package has a dependency on 0.5.0
of this library.
When running eslint
from the root of the plugin, we get the following error:
/Users/daniel-westall/sites/hmn.newsuk/vvv/www/wordpress-default/public_html/wp-content/plugins/nu-sun-web-wp-home-page-customiser-V2/vendor/humanmade/coding-standards/packages/eslint-config-humanmade/index.js:
Configuration for rule "object-curly-newline" is invalid:
Value "[object Object]" should be equal to one of the allowed values.
Value "[object Object]" should NOT have additional properties.
Value "[object Object]" should match exactly one schema in oneOf.
Value "[object Object]" should NOT have additional properties.
Value "[object Object]" should match exactly one schema in oneOf.
This version of react-script
uses eslint
v5. I couldn't find anything related to this rule specifically in the release notes
I'm guessing either the new version has some backward incompatible change that I haven't found, or there's something broken in the config of the plugin.
Also
[REACT] ./src/components/ItemClipboard.js
[REACT] Line 1: Definition for rule 'jsx-a11y/href-no-hash' was not found jsx-a11y/href-no-hash
This rule seems to be disabled in our coding standards, so that shouldn't be happening
PHPCS 3.3 fixes an issue whereby the end of comment blocks was mis-identifed if a phpcs:disable/enable rule was included in the block. This is causing false issues in a client repo (ask me for more details if necessary). We should upgrade if there are not other various issues that arise form the upgrade.
See squizlabs/PHP_CodeSniffer#1918 for more details on the specific issue that I'me seeing.
Pretty sick of WPCS changing rules out from underneath us, which is due to specifying ^0.14.0
. We should have a specific version instead.
The rule "Assignments must be first block of code on a line" reports incorrectly when using the following syntax:
$foo = $bar = 'thing';
When using the HM Linter, you cannot extend the ESLint configuration by referring to the location of the ruleset on the filesystem.
Indeed it is sufficient to do the following (with sample rule customisation):
{
"extends": "humanmade",
"rules": {
"react/react-in-jsx-scope": "off"
}
}
As the Linter does not really have a documentation page, we should include these instructions on this repository instead.
http://engineering.hmn.md/style/ do not work. Instead http://engineering.hmn.md/
I think I saw some mention of a columns length change somewhere, but when I run phpcbf
it's doing odd indenting / line returns, such as:
add_filter( 'determine_current_user', function ( $user ) {
return 1;
} );
=>
add_filter(
'determine_current_user', function ( $user ) {
return 1;
}
);
Like, what even is that?!
The WordPress.WhiteSpace.ControlStructureSpacing.NoSpaceAfterCloseParenthesis
rule is (currently) inconsistent.
I get:
Space between opening control structure and closing parenthesis is required
(WordPress.WhiteSpace.ControlStructureSpacing.NoSpaceAfterCloseParenthesis)
For:
function test2( array $foo ): \Generator {
yield 1;
}
But no complaints for the exact same code without a function parameter:
function test1(): \Generator {
yield 1;
}
It might be fixed upstream?
There's seems to be a new part of WordPress.Arrays.MultipleStatementAlignment.NotSameWarning
that makes the following a failure:
$all['read_applications'] = true;
$all['read_application'] = true;
Do we want this / is this intentional? This is pretty dumb I think in most situations and probably leads to be putting lots of daniel spacing everywhere so the lines are not consecutive.
HM.Layout.Order.WrongOrder
can throw a false positives in files with anonymous functions using use.
This code
<?php
namespace Hamilton;
use Hamilton\Mixtape;
const WRITER = 'Lin-Manuel Miranda';
add_filter( 'Hamilton/Cast', function ( $cast ) use ( $replacements ) {
return $replacements;
} );
Error thrown:
----------------------------------------------------------------------
FOUND 1 ERROR AFFECTING 1 LINE
----------------------------------------------------------------------
1 | ERROR | use found on line 8, but const was declared on line 6.
| | Statements should be ordered `namespace`, `use`, `const`,
| | `require`, then code. (HM.Layout.Order.WrongOrder)
----------------------------------------------------------------------
Directory names do not account for camel case or underscores in namespaces.
I have the file mu-plugins/pwcc-multi-domain/inc/post-types/namespace.php
PWCC\MultiDomain\PostTypes
throws an error requesting the directory name posttypes
- reportPWCC\MultiDomain\Post_Types
throws an error requesting the directory name post_types
- reportOddly mu-plugins/pwcc-multi-domain/inc/namespace.php
does not trigger an error for the namespace PWCC\MultiDomain
.
In particular, #8 introduced some more complex behaviour for the T_USE token.
When submitting a pull request, Travis CI fails.
Current error:
0.17s$ cd /tmp/phpcs && composer install
Composer could not find a composer.json file in /tmp/phpcs
To initialize a project, please create a composer.json file as described in the https://getcomposer.org/ "Getting Started" section
The command "cd /tmp/phpcs && composer install" failed and exited with 1 during .
For React rendering performance we should consider adding jsx-no-bind to this eslint config; bound functions and arrow functions created within a render
method's JSX will never be treated as equivalent when React is comparing subcomponent properties and can lead to a lot of unnecessary (and therefore expensive) re-renders.
We currently have no tests for the stylelint rules and we should add them to make sure that we're running the rules correctly and have greater confidence in changes.
I don't want to hold up #45 just for this, but we should add the tests in a subsequent PR.
These need to match the template hierarchy, so the name isn't flexible. We should exempt the following patterns from WordPress.Files.FileName.NotHyphenatedLowercase
:
category-*.php
author-*.php
taxonomy-*.php
archive-*.php
single-*.php
page-*.php
In #75 @igmoweb proposed the addition of the sort-imports ESLint rule, to control the ordering of modules we import
into our code.
In a Slack thread @rmccue further proposed that we conform to a general ordering of absolute, then relative, then side effects (e.g. importing a CSS file to ensure it gets included in the build, but not using the return of that import), with a blank line between each, and each section alphabetized.
As an example:
import React from 'react';
import { connect } from 'react-redux';
import Foo from './Foo';
import './Bar.css';
This made sense to me, and would help a lot on several projects we've had lately.
The sort-imports
rule looks like it can handle putting the import-for-side-effects after other imports, but doesn't at first glance support including the relative imports after absolute. We should investigate further.
We've had a Stylelint config sitting in this repo for almost a year now, and it would be be most useful to our project to release this and get it into practice on projects. This is a rough plan of steps involved with getting Stylelint moved forward and into use on our projects.
As with everything, would love perspectives and to know if I've missed a step here.
"wp-coding-standards/wpcs": "^0.10.0"
https://github.com/humanmade/coding-standards/blob/master/composer.json#L7
Should be 0.13.1:
https://github.com/WordPress-Coding-Standards/WordPress-Coding-Standards/releases
Getting a lot of bogus errors when running Travis CI with PHP 7.2 (nightly)
Deprecated: Function create_function() is deprecated in /home/travis/build/[secure]/[secure]/vendor/wp-coding-standards/wpcs/WordPress/Sniffs/WP/PreparedSQLSniff.php
Warning: count(): Parameter must be an array or an object that implements Countable in /home/travis/build/[secure]/[secure]/vendor/wp-coding-standards/wpcs/WordPress/Sniffs/WP/I18nSniff.php on line 234
...
This doesn't cause the behaviour we want, so we should remove it.
It seems console.error
causes a eslint warning. Is this intentional? Is doing console.error / console.warn not a valid thing to do in production code?
Currently we have:
space-unary-ops:
- error
- words: true
nonwords: true
overrides:
++: false
--: false
and we should have:
space-unary-ops:
- error
- words: true
nonwords: false
Can we remove PSR2.Namespaces.UseDeclaration.MultipleDeclarations ("There must be one USE keyword per declaration")?
Modern PHP supports this syntax, and the above rule blocks it:
use some\namespace\{ClassA, ClassB, ClassC as C};
use function some\namespace\{fn_a, fn_b, fn_c};
use const some\namespace\{ConstA, ConstB, ConstC};
The readme structure could be improved, by regrouping content.
I propose the following:
I'm unsure about the Included Checks section, because it could be interpreted as providing more background about the sniffs, or be considered a shout out section. We could integrate and extend this content based on what the goal is.
Let's discuss structure first, happy to follow up with a PR once we have settled on an outline.
The peer dependencies of our ESLint config need to be updated for ESLint 4:
warning " > [email protected]" has incorrect peer dependency "eslint@^3.8.1".
warning " > [email protected]" has incorrect peer dependency "eslint@^2.10.2 || 3.x".
warning " > [email protected]" has incorrect peer dependency "eslint@^2.0.0 || ^3.0.0".
I have WordPress-VIP installed globally, and PHPCS can find it just fine:
$ phpcs -i
The installed coding standards are MySource, PEAR, PHPCS, PSR1, PSR2, Squiz, Zend, WordPress, WordPress-Core, WordPress-Docs, WordPress-Extra and WordPress-VIP
If I then install this repo globally, e.g.
$ composer global require humanmade/coding-standards
[snipped]
$ phpcs --config-set installed_paths /Users/leewillis/.composer/vendor/wp-coding-standards/wpcs/,/Users/leewillis/.composer/vendor/humanmade/coding-standards/
Config value "installed_paths" added successfully
HM shows up as an available standard:
$ phpcs -i
The installed coding standards are MySource, PEAR, PHPCS, PSR1, PSR2, Squiz, Zend, WordPress, WordPress-Core, WordPress-Docs, WordPress-Extra, WordPress-VIP and HM
However - it's unusable:
$ phpcs --standard=HM foo.php
Fatal error: Uncaught PHP_CodeSniffer_Exception: Referenced sniff "WordPress-VIP" does not exist in /Users/leewillis/.composer/vendor/squizlabs/php_codesniffer/CodeSniffer.php:1167
Stack trace:
#0 /Users/leewillis/.composer/vendor/squizlabs/php_codesniffer/CodeSniffer.php(780): PHP_CodeSniffer->_expandRulesetReference(Object(SimpleXMLElement), '/Users/leewilli...', 0)
#1 /Users/leewillis/.composer/vendor/squizlabs/php_codesniffer/CodeSniffer.php(578): PHP_CodeSniffer->processRuleset('/Users/leewilli...')
#2 /Users/leewillis/.composer/vendor/squizlabs/php_codesniffer/CodeSniffer/CLI.php(956): PHP_CodeSniffer->initStandard(Array, Array, Array)
#3 /Users/leewillis/.composer/vendor/squizlabs/php_codesniffer/CodeSniffer/CLI.php(113): PHP_CodeSniffer_CLI->process()
#4 /Users/leewillis/.composer/vendor/squizlabs/php_codesniffer/scripts/phpcs(25): PHP_CodeSniffer_CLI->runphpcs()
#5 {main}
thrown in /Users/leewillis/.composer/vendor/squizlabs/php_codesniffer/CodeSniffer.php on line 1167
I think because of the clobbering of install paths done in HM/ruleset.xml
, but not sure what the fix would be.
file_get_contents()
and other filesystem functions are fine. WP_Filesystem
isn't appropriate 99% of the time.
We should include the rules from PHPCompatibility for PHP 7.2 in the base set of rules, to ensure all greenfields code is compatible for when we roll it out to servers.
The following code is NOT flagged as an error, but I would expect it to be. Note missing space after the first (
.
get_extended_template_part('atoms/heading', '4', [
'heading_three' => 'This is a heading 3',
] );
However it is correctly enforced if the args are on a single line. e.g.
get_extended_template_part('atoms/heading', '4', [ 'heading_three' => 'This is a heading 3' ] );
We should have real unit tests here.
In #41 we removed the object property alignment rule for JS, making this valid code:
const x = {
foo: "bar",
longerPropertyName: "value",
};
Should we do the same for PHP? Currently, this code fails:
$x = [
'foo' => 'bar',
'longerPropertyName' => 'value',
];
The error is:
Array double arrow not aligned correctly; expected 16 space(s) between "'foo'" and double arrow, but found 1.
(WordPress.Arrays.MultipleStatementAlignment.DoubleArrowNotAligned)
The following are currently failing the object-curly-newline
rules, and should not be:
import { Route, withRouter } from 'react-router-dom';
const { lastView, onDismiss } = props;
These appear to now have enforced newlines, when they should be optional.
This is done, because, I quote:
- A wrapper for PHP's parse_url() function that handles consistency in the return
- values across PHP versions.
- PHP 5.4.7 expanded parse_url()'s ability to handle non-absolute url's, including
- schemeless and relative url's with :// in the path. This function works around
- those limitations providing a standard output on PHP 5.2~5.4+.
- Secondly, across various PHP versions, schemeless URLs starting containing a ":"
- in the query are being handled inconsistently. This function works around those
- differences as well.
- Error suppression is used as prior to PHP 5.3.3, an E_WARNING would be generated
- when URL parsing failed.
As far as I can tell, parse_url()
is safe to use if you know your environment is going to be PHP >= 7.0. Does this project have a minimum supported PHP version? If it does, and it matches this constraint, can we please consider adding a rule exclude for WordPress.WP.AlternativeFunctions.parse_url_parse_url
thank you
The following causes a warning: "There must be one USE keyword per declaration"
use function NewsUK\HomepageCustomiser\{get_layout_id_for_term,get_preview_script_url,get_preview_style_url,is_enabled_for_term};
This syntax is similar to how we write import
statements in JS, so would make sense to allow
https://github.com/DealerDirect/phpcodesniffer-composer-installer is a better way of setting installed_paths
than the hack we have. Should fix #2 in the process.
We are currently whitelisting unit(less, actually)s for line-height
only in our stylelint config, but I propose that we get more specific with what units are available.
Common best practice is to use rem
units for font size based off of a single root value. We should enforce rem
units, or at a minimum, blacklist px
units to prevent them from being used.
ms
UnitsFor timing functions, such as transitions or animations, ms
units are far clearer and easy to digest than seconds because they are laid out in longform. While they mean the same thing, 400ms
is cleaner than .4s
and feels more precise in my opinion. I suggest that we enforce ms units on all timing functions.
Ref https://stylelint.io/user-guide/rules/declaration-property-unit-whitelist/ & https://stylelint.io/user-guide/rules/declaration-property-unit-blacklist/
Thoughts #teamfrontend?
cc @peterwilsoncc @sambulance @goldenapples @rmccue @joemcgill
If I run eslint --fix
, the following code...
{ foo: 1, bar: 2 },
is converted into this.
{
foo: 1, bar: 2,
},
I don't think we should allow this.
In this case I would expect this:
{
foo: 1,
bar: 2,
},
Finding props sometimes in a React file is a bit difficult due to the random order. We could consider jsx-sort-props rule to automagically sort them alphabetically.
In the same line, we could also think about adding https://eslint.org/docs/rules/sort-imports to sort imports too.
Possibly broken in #37.
Inline PHP statement must end with a semicolon
- the style guide specifically says not to do thisI think I might have seen this proposal somewhere before, but I can't find it here. Instead of requiring my_namespace/namespace.php
where your namespace doesn't have any classes, we should support my_namespace.php
so there's a little less folder cruft.
This sniff currently includes the token for interfaces, and errors, insisting that the file have a class prefix on the file name. I dont think this is correct behaviour a interface filename should be interface-foo.php
not class-foo.php
Is this intentional or a bug, if its intentional whats the reasoning behind it.
In order to better assess third-party code, we should split out the core (security, performance) rules from the style rules. We should do this for both PHP and JS, but PHP is a larger priority.
The following code should be an error. The WP coding standards include checks for unescaped output WordPress.XSS.EscapeOutput.OutputNotEscaped
echo $foo;
But our coding standards are not flagging it - this rule is explicitly ignored
Was this intentional?
Currently, we have a max-nesting
rule that is very strict at only allowing 2 levels of nesting with scss
, with the exception of blockless-at-rules
. I think it might be beneficial to loosen this just a bit for @media
rules by adding "ignoreAtRules": ["media"]
to the general rule in Stylelint.
Stylelint currently already ignores @media
at the first level in a nesting pattern when parsing for nesting depth, but it does not ignore @media
when it resides down the chain. If we make the proposed change, @media
levels will not affect the nesting depth evaluation, and we still shouldn't end up with too-specific of selectors.
Thoughts?
cc @peterwilsoncc @sambulance @goldenapples @rmccue @joemcgill @kirstyburgoine
There are currently errors on the master branch when running PHPUnit. This blocks the merge of any new PR's, as the merge button is flagged to only accept passing Travis runs. I'm opening this issue to track resolving these failures.
PHPUnit 5.7.27 by Sebastian Bergmann and contributors.
.......FFF. 11 / 11 (100%)
Time: 140 ms, Memory: 12.00MB
There were 3 failures:
- HM\CodingStandards\Tests\FixtureTests::test_passing_files with data set #1 ('/hm/gh/coding-standards/tests...er.php')
File /fixtures/pass/use-order.php should not contain any errors
Failed asserting that two arrays are equal.
--- Expected
+++ Actual
@@ @@
Array (
- 11 => Array (...)
- 15 => Array (...)
- 19 => Array (...)
- 23 => Array (...)
- 27 => Array (...)
- 31 => Array (...)
- 35 => Array (...)
- 39 => Array (...)
- 43 => Array (...)
)
/hm/gh/coding-standards/tests/FixtureTests.php:69
- HM\CodingStandards\Tests\FixtureTests::test_passing_files with data set #2 ('/hm/gh/coding-standards/tests...ce.php')
File /fixtures/pass/inc/namespace.php should not contain any errors
Failed asserting that two arrays are equal.
--- Expected
+++ Actual
@@ @@
Array (
- 11 => Array (...)
- 13 => Array (...)
- 14 => Array (...)
- 15 => Array (...)
- 16 => Array (...)
- 18 => Array (...)
- 19 => Array (...)
- 21 => Array (...)
- 28 => Array (...)
- 30 => Array (...)
- 31 => Array (...)
- 32 => Array (...)
- 33 => Array (...)
)
/hm/gh/coding-standards/tests/FixtureTests.php:69
- HM\CodingStandards\Tests\FixtureTests::test_failing_files with data set #0 ('/hm/gh/coding-standards/tests...er.php')
Failed asserting that two arrays are equal.
--- Expected
+++ Actual
@@ @@
Array (
1 => Array (...)
- 10 => Array (...)
- 16 => Array (...)
)
/hm/gh/coding-standards/tests/FixtureTests.php:118
FAILURES!
Tests: 11, Assertions: 8, Failures: 3.
Tested on Travis in #72 and locally with PHP 7.2.7
The NamespaceDirectoryNameSniff
sniff currently requires namespaces within test directory files, but this should not be required as test files are outside of a plugin-based organizational structure.
Should check for /.test
and /test
directories in path and if found, bypass this rule.
We need to use ESLint so it correctly understands JSX.
A declarative, efficient, and flexible JavaScript library for building user interfaces.
๐ Vue.js is a progressive, incrementally-adoptable JavaScript framework for building UI on the web.
TypeScript is a superset of JavaScript that compiles to clean JavaScript output.
An Open Source Machine Learning Framework for Everyone
The Web framework for perfectionists with deadlines.
A PHP framework for web artisans
Bring data to life with SVG, Canvas and HTML. ๐๐๐
JavaScript (JS) is a lightweight interpreted programming language with first-class functions.
Some thing interesting about web. New door for the world.
A server is a program made to process requests and deliver data to clients.
Machine learning is a way of modeling and interpreting data that allows a piece of software to respond intelligently.
Some thing interesting about visualization, use data art
Some thing interesting about game, make everyone happy.
We are working to build community through open source technology. NB: members must have two-factor auth.
Open source projects and samples from Microsoft.
Google โค๏ธ Open Source for everyone.
Alibaba Open Source for everyone
Data-Driven Documents codes.
China tencent open source team.