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

Potentially faulty core fixes: array indentation / array interspersed with comments #985

Closed
jrfnl opened this Issue Jun 18, 2017 · 4 comments

Comments

Projects
None yet
3 participants
@jrfnl
Contributor

jrfnl commented Jun 18, 2017

Summary:

While looking at the array indentation sniff, I realized that if there are comments between array items, the indentation of these would not be corrected.

Related #973

Example:

File: N/A - I have no doubt, we will actually run into this, but haven't come across it yet in the limited group of fixed files I've inspected so far.

Source:

get_current_screen()->add_help_tab( array(
'id'		=> 'overview',
// Here we have a comment.
'title'		=> __('Overview'),
// Here we have a comment.
'content'	=> 'value',
) );

Fixed as:

get_current_screen()->add_help_tab( array(
	'id'		=> 'overview',
// Here we have a comment.
	'title'		=> __('Overview'),
// Here we have a comment.
	'content'	=> 'value',
) );

Expected fix:

get_current_screen()->add_help_tab( array(
	'id'		=> 'overview',
	// Here we have a comment.
	'title'		=> __('Overview'),
	// Here we have a comment.
	'content'	=> 'value',
) );

@jrfnl jrfnl added the Type: Bug label Jun 18, 2017

@GaryJones

This comment has been minimized.

Show comment
Hide comment
@GaryJones

GaryJones Jun 18, 2017

Member

Not sure if it's applicable here, but tests may also want to cover a DocBlock as well as single line comments i.e.

$my_array = [
	/**
	 * Documentation about a filtered value...
	 */
	'truthy' = apply_filters( '...', true );
]
Member

GaryJones commented Jun 18, 2017

Not sure if it's applicable here, but tests may also want to cover a DocBlock as well as single line comments i.e.

$my_array = [
	/**
	 * Documentation about a filtered value...
	 */
	'truthy' = apply_filters( '...', true );
]

@jrfnl jrfnl changed the title from Potentially fault core fixes: array indentation / array interspersed with comments to Potentially faulty core fixes: array indentation / array interspersed with comments Jun 19, 2017

@ntwb

This comment has been minimized.

Show comment
Hide comment
@ntwb

ntwb Jun 19, 2017

Member

Via https://make.wordpress.org/core/handbook/best-practices/inline-documentation-standards/php/#5-2-multi-line-comments

"Important note: Multi-line comments must not begin with /** (double asterisk) as the parser might mistake it for a DocBlock. Use /* (single asterisk) instead."

So, updating the example above so that is not detected as a DocBlock:

$my_array = [
	/*
	 * Documentation about a filtered value...
	 */
	'truthy' = apply_filters( '...', true );
]
Member

ntwb commented Jun 19, 2017

Via https://make.wordpress.org/core/handbook/best-practices/inline-documentation-standards/php/#5-2-multi-line-comments

"Important note: Multi-line comments must not begin with /** (double asterisk) as the parser might mistake it for a DocBlock. Use /* (single asterisk) instead."

So, updating the example above so that is not detected as a DocBlock:

$my_array = [
	/*
	 * Documentation about a filtered value...
	 */
	'truthy' = apply_filters( '...', true );
]
@jrfnl

This comment has been minimized.

Show comment
Hide comment
@jrfnl

jrfnl Jun 19, 2017

Contributor

@ntwb Sniffs should function independently of each other. So both those comment styles should be detected.

P.S.: both the above examples have a parse error: = should be => and the ; should be ,.

FYI: I've been working on a fix for this and #973 and believe I have got this sorted, but running into some issues with differences in the end results between the unit tests and manual runs.

Contributor

jrfnl commented Jun 19, 2017

@ntwb Sniffs should function independently of each other. So both those comment styles should be detected.

P.S.: both the above examples have a parse error: = should be => and the ; should be ,.

FYI: I've been working on a fix for this and #973 and believe I have got this sorted, but running into some issues with differences in the end results between the unit tests and manual runs.

@ntwb

This comment has been minimized.

Show comment
Hide comment
@ntwb

ntwb Jun 19, 2017

Member

Right, thanks @jrfnl, still much to learn in the art of sniffing 😏

Member

ntwb commented Jun 19, 2017

Right, thanks @jrfnl, still much to learn in the art of sniffing 😏

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