search

sirbrillig / phpcs-changed

🐘 Run phpcs on files and only report new warnings/errors compared to the previous version.
MIT License
31 stars 11 forks source link

Add emacs report type for use with vim plugins #35

Closed delputnam closed 1 year ago

delputnam commented 4 years ago

This adds the "emacs" report type so that it can be used with vim plugins such as "ALE".

When using vim plugins that report code sniff errors, it is nice to be able to limit the errors reported to just the lines that have been changed in certain cases to reduce the noise on some older files. Many of these plugins allow alternate executables to be specified, so adding this report type to phpcs-changed would allow this.

delputnam commented 4 years ago

@sirbrillig I'm not sure if there is a better way to handle the use of the --stdin-path option. The way I've done it is basically a hack that makes this work for ALE at least, but from my first glance at the code, it looked like making this work properly might require a very large PR, so I thought I'd start with this and then see if you have any suggestions.

sirbrillig commented 4 years ago

This looks great! Thank you, @delputnam. I just want to make sure we document the new emacs reporter in the README, and that we also document the new --stdin-path option. Could you add that?

I guess I'm not 100% sure I understand what that option is supposed to do in phpcs... it provides STDIN from a file, but if that's just for picking which file to use as input, why does the option exist at all? (I understand that you're trying to mimic the existing behavior of phpcs, I'm just trying to grok why it's there.)

delputnam commented 4 years ago

I just want to make sure we document the new emacs reporter in the README, and that we also document the new --stdin-path option. Could you add that?

I can add the additional documentation...I actually thought about it and somehow forgot to do it. 🤦

I guess I'm not 100% sure I understand what that option is supposed to do in phpcs... it provides STDIN from a file, but if that's just for picking which file to use as input, why does the option exist at all? (I understand that you're trying to mimic the existing behavior of phpcs, I'm just trying to grok why it's there.)

So, I think that what the --stdin-path is actually supposed to do is provide the file name to use when comparing the data read from STDIN to the contents of a file in subversion. This would be useful, for example, when trying to generate a diff from the contents of a buffer that hasn't yet been saved to disk.

For example, when ALE runs phpcs, it sets this value to the name of the file it's generating output for and also provides the contents of the buffer to phpcs on STDIN.

As you can see, that's not exactly how I'm using this option in this diff which is why I said it was kind of hacky. In my case, generating the error list on save was "good enough" and it seemed from my reading of the code that it might take some major re-engineering to actually read the input from STDIN and properly set the file names to only be used for generating the diffs. But if you are willing to give me some pointers, I'm willing to give it a shot. :-)

sirbrillig commented 3 years ago

Just a ping to see if this is still in-progress. Seems like a valuable addition! Sorry I hadn't remembered about it until now.

it seemed from my reading of the code that it might take some major re-engineering to actually read the input from STDIN and properly set the file names to only be used for generating the diffs. But if you are willing to give me some pointers, I'm willing to give it a shot.

I'm happy to help however I can! I'd say that the first thing to do would be to write a test to simulate the behavior you're trying to achieve (even if it might be missing some mocks). If you'd like to get this change merged sooner though, it seems like the "hacky" version is better than nothing and this does provide something useful. I'm happy to merge this basically as-is (with the new options documented). I'll let you decide.

sirbrillig commented 1 year ago

Closing as abandoned.