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

Update Core to comply to coding standards #8

Closed
wants to merge 1 commit into from

Conversation

@pento
Copy link
Member

commented Nov 23, 2017

See the Trac ticket.


<!--
phpcbf -.-standard=phpcs8.xml.dist -.-report-summary -.-report-source -.-report-full=./20170926-11-phpcbf-all-incl-new-sniffs.txt -.-basepath=I:/000_GitHub/WP_test_suite/
-->

This comment has been minimized.

Copy link
@ntwb

ntwb Nov 23, 2017

Member

The above 3 line comment should be removed, or updated to reference a suggested command line example, e.g. phpcbf --standard=phpcs.xml.dist --report-summary --report-source

'https://github.com/WordPress/gutenberg' ); ?></p>
'https://github.com/WordPress/gutenberg'
);
?>

This comment has been minimized.

Copy link
@ocean90

ocean90 Nov 23, 2017

Member

The indent doesn't seem right here.

wp_redirect( admin_url( 'upgrade.php?_wp_http_referer=' . urlencode( wp_unslash( $_SERVER['REQUEST_URI'] ) ) ) );
exit;
/**
/**

This comment has been minimized.

Copy link
@ocean90

ocean90 Nov 23, 2017

Member

Nope.

This comment has been minimized.

'timeout' => 120,
'httpversion' => '1.1',
)
);

This comment has been minimized.

Copy link
@ocean90

ocean90 Nov 23, 2017

Member

What rule are we following here? Should this be more like this:

$response = wp_remote_get(
	admin_url( 'upgrade.php?step=1' ),
	array(
		'timeout'     => 120,
		'httpversion' => '1.1',
	)
);

This comment has been minimized.

Copy link
@pento

pento Nov 23, 2017

Author Member

Yeah, that looks wrong. Can you report it to the WPCS repo?

This comment has been minimized.

Copy link
@ocean90

ocean90 Nov 27, 2017

Member

I can, but I wasn't sure if that's a rule we have because it's all over the code base now. Or should I create one for discussion first?

This comment has been minimized.

Copy link
@pento

pento Nov 27, 2017

Author Member

If it doesn't look right, it's probably a bug. ;)

$id, array(
'send' => false,
'delete' => true,
)

This comment has been minimized.

Copy link
@ocean90

ocean90 Nov 23, 2017

Member

See above, another example:

echo get_media_item(
	$id,
	array(
		'send'   => false,
		'delete' => true,
	)
);
@@ -14,7 +14,7 @@
// Back compat hooks
if ( 'category' == $taxonomy ) {
/**
* Fires before the Edit Category form.
* Fires before the Edit Category form.

This comment has been minimized.

Copy link
@ocean90

ocean90 Nov 23, 2017

Member

This probably needs manual fixing, there's actually a space before the tab but now it's after the tab. 🤔

This comment has been minimized.

filters.slideUp('fast');
switch ( $(this).val() ) {
case 'attachment': $('#attachment-filters').slideDown(); break;
case 'posts': $('#post-filters').slideDown(); break;
case 'pages': $('#page-filters').slideDown(); break;
}
});
});

This comment has been minimized.

Copy link
@ocean90

ocean90 Nov 23, 2017

Member

Same as above, the extra whitespaces don't get removed, just moved after the tab.

<?php
/* translators: %s: importer slug */
printf( __( 'The %s importer is invalid or is not installed.' ), '<strong>' . esc_html( $_GET['invalid'] ) . '</strong>' );
?>

This comment has been minimized.

Copy link
@ocean90

ocean90 Nov 23, 2017

Member

That's too much indentation.

@@ -9,7 +9,7 @@
*/
class StringExtractor {

var $rules = array(
var $rules = array(

This comment has been minimized.

Copy link
@ocean90

ocean90 Nov 23, 2017

Member

Not sure if we have to update tools/i18n/ here too. 🤷‍♂️

Did you run the tests manually?

This comment has been minimized.

Copy link
@pento

pento Nov 23, 2017

Author Member

That's the expected behaviour, but it looks a bit weird. Move the $comment_prefix declaration above $rules, or add a blank line between them.

Run what tests manually?

This comment has been minimized.

Copy link
@ocean90

ocean90 Nov 23, 2017

Member

I meant the whole tools/i18n directory.

The tests in https://core.trac.wordpress.org/browser/trunk/tools/i18n/t.

This comment has been minimized.

Copy link
@pento

pento Nov 23, 2017

Author Member

Given that I've never opened that directory before... no. Why doesn't it run in Travis?

This comment has been minimized.

Copy link
@ocean90

ocean90 Nov 27, 2017

Member

I was never a fan of having that directory on core.trac at all. For me the canonical source is still https://i18n.trac.wordpress.org/browser/tools.

This comment has been minimized.

Copy link
@dd32

dd32 Nov 27, 2017

Member

Let's just external one of them.

@pento pento force-pushed the pento:41057-coding-standards branch from 313b7c3 to 7d9dc4f Nov 30, 2017
@pento pento closed this Dec 1, 2017
@pento pento deleted the pento:41057-coding-standards branch Dec 1, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.