-
-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
Feature Request: Line Number with Checkstyle Format #3601
Comments
when the formats were introduced one after another (last one - checkstyle indeed), there were no good differ available. Thanks to amazing job of @SpacePossum, we can now generate diff in udiff format. We are already prepared that checkstyle format could contain linenumber (vide https://github.com/FriendsOfPHP/PHP-CS-Fixer/blob/2.10/doc/checkstyle.xsd#L30), we were thinking about it even without having good differ. Would you like to update https://github.com/FriendsOfPHP/PHP-CS-Fixer/blob/2.10/src/Report/CheckstyleReporter.php to expose line ? :) |
Sure thing, I'll take a crack at it. |
Currently the check style report contains entries for each fixer, this makes the line number not useful in some situations. For example one fixer may change a series of tokens on line 2 and than the next fixer moves the tokens to line 20 or removes all tokens leaving no "line 2" in the file. Not sure how to proceed here in a meaningful matter. |
I think at least when --dry-run is used, the line number the error is on is all that matters since fixers are not run. When one issue is fixed, the line numbers the errors were on will obviously change. |
If you run with just one rule, than yeah the line numbers will always map back to that one rule and you should be good :) If you run multiple rules (fixers) you still run into the same issue, in dry-run or not, as the fixed result from the first rule is the input of the next, of which the fixed/changed output is the input of the 3rd etc. |
Right, I was specifically just talking about when doing --dry-run which doesn't modify code. Having the line numbers here at least would be useful for CI. We don't automatically fix code, just validate. I'm not sure if doing it for --dry-run only for now, and then doing this for the default mode would be possible. |
And this is a misconception on your part: So, as SpacePossum said, the fixed result from the first rule is still the input of the next, etc. Because otherwise it would not be a representative dry run. |
Ah ok. I had no idea --dry-run actually modified anything. I thought it just output what would have been changed. Is this what other linters do (eslint, prettier, etc.)? I ask because I actually came to this ticket as I was having issues with https://github.com/haya14busa/reviewdog and I think it's due to the missing line numbers. It depends on having the line number in the error match up with the line number in the git diff. If |
The only way to know this is to actually do the fixing (for all configured rules), and diff that with starting code. That is usually how dry runs work: do everything you'd normally do, just don't save (write, commit, publish, etc) the end-result. However as I understand correctly (haven't used it that way myself), you can specify the diff format to output just regular diffs, and those should have line numbers etc. You then just can't speak as to what specific fixer caused which specific change (unless only 1 fixer actually made changes).
PHP-CS-Fixer doesn't work on a line by line basis (luckily, otherwise a lot of fixers wouldn't be possible), so if this is necessary, it won't ever work. Take for example this code /**
* @param int $foo
*/
function bar($foo); If your change (and thus git diff) would be /**
* @param int $foo
*/
-function bar($foo);
+function bar(int $foo); And your fixer config includes function bar(int $foo); Making the PHP-CS-Fixer diff -/**
- * @param int $foo
- */
function bar(int $foo); How could you map that to your git diff? |
I would like to add to the discussion to suggest an alternate approach to solving this problem. Since While I am not an expert on the code base, it would stand to reason that the same logic that is used when running the fixers on rule violations could also be used to capture the file, line, and rule violation (minus running the fixers). If it's possible that some rule violations might be masked by other rule violations, then so be it -- the user/system can re-run |
running
That's the thing, to know which line is violating the rule, the rule has to be executed on a file. executing the rule = fixing. Executing without store fixed output = dry-run mode. |
Yes, and the suggestion was intentional. The suggestion is two-fold:
Yes,
I took the liberty of forking the code earlier today and I poked around a little bit to see what it would take to add line numbers to the output. I do understand what you're saying about having to run the fixers to know the line numbers. I still believe this feature is trivial to implement; albeit, it will mean modifying each existing fixer, plus a handful of other files, to get it to work. Here is what I was thinking:
array(
'relative/path/to/file.php' => array(
'appliedFixers' => array(
'fixer-1', 'fixer-2',...
),
'diff' => '... diff content ...'
)
);
Granted, I am still new to this code base, so I could have missed something, or you might have a better/cleaner idea than what I've laid out. These are just cursory thoughts based on an afternoon digging around the code. |
👎 for modifying Also, you're proposing to modify every single fixer to add a new method to report line number violations - which would be a hell of a lot of work, and that's without considering testing on top of that. I haven't thought about this long and hard, but I was thinking possibly a decorator for |
You need to modify the code in memory, as if you won't, not all changes will be detected (there are changes that would be applied only if other fixer would be applied earlier)
I'm really eager to hear who gonna implement this method for built-in 200 fixers and more in the wild
Imagine single violation that private method should be moved after public methods. Now, we got 500 tokens moved around, but we shall have it reported only as single violation we already have a differ that can produce line numbers, why not simply use it ? |
Yes, but who else in the codebase aside of the fixers has the knowledge to know when a token is also a violation? With a new interface or modifying an existing interface, there are going to be roughly 200 classes either added or modified due to the way the existing infrastructure works. In other words, the fixers are tightly coupled with the logic used to determine what's a violation.
If it were easy, we probably wouldn't be having this discussion; as the line numbers would probably already be available in the summary reports. In other words, we shouldn't let that stop us. :)
Are there already use cases that demonstrate violations that are hidden due to other violations? I'm hard-pressed to think of any good use case. Let me counter your argument by using the analogy of the built-in PHP linter/lexer: it does not try to guess at every possible problem in the code -- it finds the first one and halts. If that problem happened to be hiding another problem, then the user is expected to re-run the code so it can be discovered. Don't get me wrong; I'm not suggesting we halt on the first problem we uncover. I'm simply suggesting that, for the sake of complexity (and speed), we not spin our wheels trying to uncover every single possible problem that could be lurking in the depths of somebody's code. Besides, there will come a point when doing so overshoots the goal of providing line numbers. For example, if a fixer would sort methods alphanumerically (irrespective of visibility), only to violate a rule about ordering by visibility, you wouldn't flag the violation on the new line numbers that happen after the original sorting. You would flag the violation at the original line numbers and move on - no modification needed.
What I'm proposing is not about fixing the code. It's only about reporting violations that are found on the existing code as a report. Fixing code is complex, and as you and a lot of others allude to, there really is no good way to get around how complex and 3-dimensional fixing code can be. So instead, we simply report on what we can reasonably report on and leave the fixing up to the user. I'll be honest: my goal is to allow
I volunteer as tribute to help with
If a class has 500 methods that violate a rule, then yes, we should have 500 violations. I would argue that the number of violations is not what should concern us, it's the quality of the violations. For example, if your class has 2
Never should we suggest all three methods are in violation. It could be as simple as a hard-fast rule that public methods are never in violation, or it could be more dynamic as to suggest whichever visibility of method has the fewest violations is the visibility that is not in violation. So in our given example, the public method would be considered correct while the private methods are in violation.
Because the differ works on modified code, and that modified code could have any number of fixers applied to them. How would you know with any level of certainty which fixer was applied to any given line of code from the original source? All you would know is that line N in path/to/file.php has a violation. Now we're back to square one, but with a different problem: what was the violation? I would argue that separating out the logic that detects the violations from the logic that fixes the violations would better serve this project going forward. I never said it would be easy, and I certainly don't expect it to happen over night. Though that would be nice. :) |
So, again. That's the whole point. PHP CS Fixer had been built as... FIXER. It does not look for violations. There is no code for that. We don't have the logic to find violation as standalone code - it's part of fixing functionality that cannot be easily extracted. And rewriting a 200 rules to do so is way we will not take.
Awesome ! Then... do it ! PHP CS Fixer in dry-run mode does exactly that. Symfony, Zend, Yahoo are using it. Scrutinizer CI offers it out of the box, ppl are using it across Travis, CircleCI, Jenkins, Bamboo... your goal has been already achieved.
then let me help with that. separating logic of detecting vs fixing for existing codebase would take few months of full-time job for any of us. are you willing to commit that deep? if so - it's wrong, dev shall be lazy and think about simpler solution.
500 tokens are not 500 methods. in my example we had a single method to be moved, and that method is defined by 500 tokens. That was killer point for ntzm's idea of wrapping Tokens.
Differ works on whatever one would run it against. You don't need to run differ after each of fixer has been applied.
This one is way bigger topic than lines reporting. Please give the reasoning for that. |
PHP was never meant to be object oriented. There never was any code for that, until two university students - against the advisement of their professor - came along and rewrote PHP's internal engine, which paved the way to PHP as we know it today. I knew a guy who once bragged back in the late 90's how UNIX was never meant to be used with a mouse, and yet he had successfully set up With all due respect, and I'll be frank here, your response is cowardly and short-sighted. Which leads me into...
I've done it. And all of my reports say each violation happens at line 0 of each file. So now my team, as well as anyone else who does this, has to spend the majority of their efforts skimming the code trying to find each and every violation, no matter how subtle. Again, with all due respect and brutal honesty: if it weren't for the breadth of the ruleset, I'd be using So, tell me where would I begin with the following violation?
Yes, I would, as long as it's the right thing to do.
I'm 100% open to suggestions. For example, I liked @ntzm's suggestion about adding a new interface that extends |
So, from the very beginning.
That's exactly how the tool was not designed to be used like. Why you ask your team to manually do it? let them run
Already did, that's what we brought |
I appreciate that. But what I don't understand is this stonewall against evolving the software beyond its original intentions; especially since I'm offering to help with the work. I can't imagine there is no room for compromise on this point.
So that they learn the coding standards and not commit poor code quality in the first place. This keeps commit logs focused on meaningful code and not the noise from poor code quality. This is especially important in code reviews, which we do very regularly with every PR. Again, I chose
Which, by the way, the line numbers are made available from Here are a couple of screenshots of what the checkstyle report looks like in Jenkins. As you can see, the diff feature is incompatible in this case. OverviewNew Tab |
I for one would also really like to know which lines has been altered by which fixer. I know I can diff afterwards, but if more fixers has fixed issues, it is no longer clear which fixer has done what. Attaching the line number to a token during the tokenization would be sufficient in most cases, since you would be able spot it in the diff anyway. |
I don't have the energy to work with this |
Please re-open. What reason do you have for closing this ticket? |
lock was accidental and Space already unlocked it 20 minutes before your request. to fast finish the talk about "how" to achieve it. https://blog.jetbrains.com/phpstorm/2018/09/phpstorm-2018-3-early-access-program-is-open/ |
I appreciate the lock being lifted. I was referring to resetting the ticket status from Closed to Open.
That's really cool. I love seeing the cross-compatibility between projects such as this. However, that side-steps the original request: add line numbers to checkstyle report. |
thank you for pointing this out... #3601 (comment) |
I somehow missed this. I will see what I can do about tracking line numbers, taking into account fixer modification concerns already discussed. In the meantime, would you or @SpacePossum reopen this ticket? Or would a new ticket be preferred? Thank you for talking this through with me. It has helped. :) |
I'm pretty convinced this ticket should be kept open. |
Not sure what the current status is, but I'd also +1 the request for line numbers. I'm currently looking into integrating code style tools into our dev process, starting from the IDE (PHPStorm in our case), pre-commit hooks (via grumphp) to continous integration via jenkins. Especially the PHPStorm integration brought me here, because the inspections are currently rather not useful, if there is more than one violation. I believe this is due to the way that php-cs-fixer reports violations. To make things more clear, here is a concrete example: <?php
//hello
function foo_bar($baz=null, $bar)
{
echo "foo"; echo "bar";
} The file contains multiple violations and this is how PHPStorm reports those violations with php-cs-fixer configured: The arrow-position of the bubble is "my cursor" and I get a list of all style violations. Unfortunately "I don't now" which of those actually applies to the given line. The command that PHPStorm runs behind the scenes is
Generating the following output:
As you can see, there is no information about "where which violation occured", but only "which fixers got applied". So I guess there's nothing that can be done from the PHPStorm side. In comparison, this is the same file but with a PHPCodeSniffer inspection instead of php-cs-fixer The error message is actually taylored to the style violation that affects the specific line in question. Command run bv PhpStorm
Output
I've two concrete examples why this is useful:
Nr. 1 has already been explained here. As for 2... well, that's my current pain point :) I'm trying to understand each rule/fixer by applying them to our existing codebase and getting (unsurprisingly) a lot of violations. Since I have no clue which violations refers to which line in the code, exploring those rules is very cumbersome, because I can't just "hover" over the violation in PHPStorm to understand what the actual issue is and look it up in the documentation. I am aware that php-cs-fixer is.. well.. a fixer and not a style checker. However, from an end user perspective I would like to use one tool and define one ruleset instead of maintaining multiple (incompatible) ones. php-cs-fixer is already integrated in our favorite IDE and offers a ton of great options, so this would be a perfect fit. PS: Thx for the discussions in here - especially @dohpaz42: Very well explained. i hope you didn't move on already (or if so... to what ;)). |
Can we have this reopened? |
This PR was squashed before being merged into the 2.15-dev branch (closes #4288). Discussion ---------- Add Gitlab Reporter Gitlab does support a subset of the CodeClimate JSON Format. I initially called the Reporter "CodeClimate", but that would "require" more fields that we currently cannot fill appropriately from a report generator, I think. So I decided to rename it to "Gitlab Reporter". It will result in a pull request annotation like this: ![image](https://user-images.githubusercontent.com/156839/52041883-f8dc5c00-253b-11e9-9f04-07faa7eaa351.png) I will be happy to maintain this reporter, if problems arise. In a next step I would be happy to add meaningful line numbers, if someone can assist me on that, but after reading #3601 it does not seem to be so easy, and I think the current state is enough for an "MVP" ;-) Commits ------- 5b54f5c Add Gitlab Reporter
Maybe some
|
Closing as the PR has to many ideas that deserve each their own issue and something have already changed in the last 2 years. |
I'd love if this tool could be used to flag errors for developers to fix, rather than fixing them automatically (in the interest of teaching junior devs to format code properly the first time, rather than rely on an automated tool to catch it for them). We can sort of do that now by executing
php-cs-fixer fix ./ --dry-run --diff
, but the given output is very large and doesn't actually identify what the error was, just the change that was made. In a perfect world I'd be able to writephp-cs-fixer fix ./app --dry-run --diff > output.txt
and see a much more truncated list with the filename, line number, and rule that is being violated.I'm aware of one other such tool, CodeSniffer, that would offer me (sort of) this functionality, but in all honesty I far prefer this package's configuration method.
EDIT: on further review, the
--format=checkstyle
option almost gives me what I want -- my only issue is that I can't get the line number with each item.The text was updated successfully, but these errors were encountered: