inpsyde / php-coding-standards

Style guide for writing consistent PHP for WordPress projects.
MIT License
98 stars 23 forks source link

LineLengthSniff returns "Line 0" instead of line number in the warning message #42

Closed meszarosrob closed 3 years ago

meszarosrob commented 4 years ago

1.0.0-beta.7 returns the "Line 0 exceeds 100 characters" warning message instead of the actual line number.

For example this code that was located on line 86

$script = (new Script(self::HANDLE_EDITOR, $assetUri . "$fileName.js", Asset::BLOCK_EDITOR_ASSETS))->withFilePath($assetDir . "$fileName.js")

returns this message

FILE: AssetProvider.php
-----------------------------------------------------------------------------------------------------------------
FOUND 0 ERRORS AND 1 WARNING AFFECTING 1 LINE
-----------------------------------------------------------------------------------------------------------------
 86 | WARNING | Line 0 exceeds 100 characters; contains 142 characters.
-----------------------------------------------------------------------------------------------------------------
gmazzap commented 4 years ago

Just to clarify: it is correct that the message it is shown (because the line is longer than max), but it is wrong that Line 0 is used in place of Line 86 . Right?

meszarosrob commented 4 years ago

Yes, in this case, the Line 0 part is wrong. The actual line is longer than 100 chars, so the warning is right about that.

gmazzap commented 4 years ago

@meszarosrob I fixed the bug.

While reviewing the code I found there was another issue, line with length equaling exactly the line limit were triggering a warning, I fixed that as well.

Moreover, I refactored the overall calculation, to make it faster and also fix some incosistencies.

For example, lines of 101 to 103 characters (assuming default limit to 100) would not trigger a warning. Now they do, so it is possible that lines that did not triggered an erro before do i now.

Finally, I included an exception to line limit, it regards inline HTML. You can see the login in this fixture file: https://github.com/inpsyde/php-coding-standards/blob/master/tests/fixtures/line-length.php#L5-L13

Basically, lines containing HTML code, with a single argument that is already over the line limit will not trigger errors, because splitting multiple attributes one per line is easily doable, splitting a single attribute just to don't trigger PHPCS warnings does not really make sense.

Can you please test latest commit? If everything is ok also for you I'll add a new tag.

meszarosrob commented 4 years ago

@gmazzap sure, I'll test it 👍 thanks a lot in advance for the fix and the improvements

meszarosrob commented 4 years ago

@gmazzap seems okay now! 💯

gmazzap commented 4 years ago

@meszarosrob I did some additional refactoring, cleaning up, added tests because I would like to prepare for stable release.

Can you please test that this is still fine with latest commit to master?

meszarosrob commented 3 years ago

Closing this as, since the last fixes, I did not run into this problem anymore.

gmazzap commented 3 years ago

Thanks @meszarosrob