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

Generator vs new phpcs.xml.dist file #54

Open
jrfnl opened this issue Jun 22, 2017 · 6 comments
Open

Generator vs new phpcs.xml.dist file #54

jrfnl opened this issue Jun 22, 2017 · 6 comments
Labels

Comments

@jrfnl
Copy link

jrfnl commented Jun 22, 2017

I haven't checked this yet, but _s now contains a phpcs.xml.dist file with references to the name _s as well, so I'm wondering if the generator handles this case.

See: https://github.com/Automattic/_s/blob/master/phpcs.xml.dist#L36

@davidakennedy
Copy link
Contributor

@jrfnl It doesn't! Good catch. We should adjust that.

@jrfnl
Copy link
Author

jrfnl commented Jun 23, 2017

Hi @davidakennedy I've just looked over your commit and it looks like you are excluding the phpcs.xml.dist file now from the download.

To be honest, I would like to suggest to include it in the download and to adjust any references to _s in the file
It is a great way to introduce the concept of coding standards to people who may have never heard of it and the phpcs.xml.dist file is by now a great example of a typical ruleset for a theme.

@davidakennedy davidakennedy reopened this Jun 23, 2017
@davidakennedy
Copy link
Contributor

Ah, my fault @jrfnl! I thought that's what you wanted to do here. Will add back, as I agree it could be good to expose people to from an education standpoint.

We should maybe add a comment at the top of the file explaining what it's for, and how themers could use it?

davidakennedy added a commit that referenced this issue Jun 23, 2017
* Also, removes unneeded `rel="designer"` attribute.

See #54
@jrfnl
Copy link
Author

jrfnl commented Jun 25, 2017

@davidakennedy No problem.

We should maybe add a comment at the top of the file explaining what it's for, and how themers could use it?

That would be a PR to make in _s presumably ?
The file does already contain lots of links at the top of the file to where people can find all the info they'd want about this.

As for the replacement issue:

I've just done a test download and the original issue why I opened this ticket is still there:

The _s in the below snippet is not being replaced by the generator at this time.
Once WPCS 0.12.0 comes out, a second customization will be added, which will also take _s as an property value, so let's keep this issue open until this is handled correctly.

	<rule ref="WordPress.WP.I18n">
		<properties>
			<property name="text_domain" type="array" value="_s" />
		</properties>
	</rule>

@redefinered
Copy link

how to use this file? I already installed code sniffer via composer

@jrfnl
Copy link
Author

jrfnl commented Apr 9, 2018

@redefinered Go to the root of your project and try this: vendor/bin/phpcs.
If all is good, you should get a clean bill of health, if not, you will see a list of errors/warnings.

Mind: as this issue has not been fixed yet, you will probably need to adjust the text-domain in your local copy of the phpcs.xml.dist file.

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

No branches or pull requests

3 participants