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

Remove the textdomain from block library #12215

Merged
merged 4 commits into from Nov 22, 2018

Conversation

Projects
None yet
5 participants
@notnownikki
Member

notnownikki commented Nov 22, 2018

Fixes: #12167

@notnownikki notnownikki added this to the WordPress 5.0 milestone Nov 22, 2018

@notnownikki notnownikki requested a review from WordPress/gutenberg-core Nov 22, 2018

@gziolo

gziolo approved these changes Nov 22, 2018

Nice one, thanks 👍

@notnownikki

This comment has been minimized.

Member

notnownikki commented Nov 22, 2018

This fails the lint checks though, do we need to remove that check?

@gziolo

This comment has been minimized.

Member

gziolo commented Nov 22, 2018

@gziolo

This comment has been minimized.

Member

gziolo commented Nov 22, 2018

Let me check what we do in core, maybe we should replace default there instead.

@notnownikki

This comment has been minimized.

Member

notnownikki commented Nov 22, 2018

Yeah, I'm just thinking that if no domain is the norm from now on because this is going to be merged into core, then we should get rid of the rule altogether, rather than having to have a lint comment every time?

@swissspidy

This comment has been minimized.

Member

swissspidy commented Nov 22, 2018

Please see this discussion in Slack: https://wordpress.slack.com/archives/C02QB2JS7/p1542804335045200

For strings that are already in core and not needed in the plugin, adding and exclusion to the phpcs config makes sense. So in this case here I'd exclude that file.

Yeah, I'm just thinking that if no domain is the norm from now on because this is going to be merged into core, then we should get rid of the rule altogether, rather than having to have a lint comment every time?

No. Not every string here in the plugin is also in core.

@gziolo

This comment has been minimized.

Member

gziolo commented Nov 22, 2018

I would rather exclude the rule in the files where it makes sense using code comment.

@notnownikki

This comment has been minimized.

Member

notnownikki commented Nov 22, 2018

Thank you both :) I'll exclude with comments.

notnownikki added some commits Nov 22, 2018

@pento

This comment has been minimized.

Member

pento commented Nov 22, 2018

@notnownikki: You should be able to exclude it by adding this to the phpcs.xml.dist file:

<rule ref="WordPress.WP.I18n.MissingArgDomainDefault">
	<exclude-pattern>packages/block-library/src/*</exclude-pattern>
</rule>
@pento

Feel free to dismiss this block once the // phpcs:ignore comments are replaced with the phpcs.xml.dist config. 🙂

@gziolo

This comment has been minimized.

Member

gziolo commented Nov 22, 2018

Cool, thanks Gary for a great hint. Let's fly with that.

@notnownikki

This comment has been minimized.

Member

notnownikki commented Nov 22, 2018

Great, have replaced the comments :)

Could someone dismiss the block for me? I can't do that.

Fixed

@gziolo gziolo modified the milestones: WordPress 5.0, 4.6 Nov 22, 2018

@notnownikki notnownikki merged commit abf1b56 into master Nov 22, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@notnownikki notnownikki deleted the update/block-library-no-text-domain branch Nov 22, 2018

youknowriad added a commit that referenced this pull request Nov 29, 2018

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