search

fidian / git-started

A good starting point for projects. Sets up lint checking, pretty printing. Easily extensible.
Other
6 stars 4 forks source link

Lint-Check and Pretty Printing #18

Closed wjaspers closed 10 years ago

wjaspers commented 10 years ago

Should the lint checker run after pretty printing? Or should it run before AND after pretty printing? I've actually encountered the pretty printer (not caused by git-started) hosing code on more than one occasion. Just looking for feedback.

fidian commented 10 years ago

It should run both before and after pretty printing. The linter should pass on the pretty printed results before the file is moved on top of the old file.

Is that not what's actually happening?

Tyler Akins

On Thu, Sep 18, 2014 at 12:46 PM, Will notifications@github.com wrote:

Should the lint checker run after pretty printing? Or should it run before AND after pretty printing? I've actually encountered the pretty printer (not caused by git-started) hosing code on more than one occasion. Just looking for feedback.

— Reply to this email directly or view it on GitHub https://github.com/fidian/git-started/issues/18.

wjaspers commented 10 years ago

It is doing it twice, but the pretty printer's changes are appear to be replacing the staged file. It's task exits cleanly, so the changes are accepted and the linter runs, then reports the error(s). I need to do a little more debugging so I can get you the actual debug output...

wjaspers commented 10 years ago

Here's a gist containing the debug output, the staged diff, and the diff after the first commit attempt (current against the staged file).

https://gist.github.com/wjaspers/a476279cf4331de3afc6

It appears the linter runs after pretty printing is finished, but the file has already been replaced with the faulty code.

fidian commented 10 years ago

Thanks for all of that useful detail. It looks like the lint checking does not happen after pretty printing. I must have chosen to not do that because of speed with various pretty printers. However I do have the ability for you turn that on. Try adding this line to your util/config/git-started-setup file. You can also do a similar change (adding +lint) to the rest of the file types that you are processing.

PRETTY_PHP="phpcsfixer+wsend+lint phpbeautifier+wsend+lint expand+wsend+lint wsend+lint"

I've updated git-started to fix a minor bug I noticed while reading the output you put into the gist. Check out commit 4f69164 for details.

I would try to just use phpcsfixer --fixers=-psr0 manually on the file in question and see if the pretty printer breaks the file. If so, that sounds like a bug in phpcsfixer.

Let me know how this turns out. Leaving the issue open for now.

wjaspers commented 10 years ago

Well I do know the PHP-CS-Fixer had a bug in it, which I've opened a PR for. Thanks for your feedback/help, too!

fidian commented 10 years ago

If you add "+lint" to the end of the chain of tools, did that work for you? Is this issue resolved?

Tyler Akins

On Mon, Sep 22, 2014 at 11:24 AM, Will notifications@github.com wrote:

Well I do know the PHP-CS-Fixer had a bug in it, which I've opened a PR for. Thanks for your feedback/help, too!

— Reply to this email directly or view it on GitHub https://github.com/fidian/git-started/issues/18#issuecomment-56399410.

wjaspers commented 10 years ago

It did work, thank you very much.

Your commit isn't pushed, though.

fidian commented 10 years ago

Happy to help.

Don't forget, we're still on IRC.

Tyler Akins

On Mon, Sep 22, 2014 at 12:45 PM, Will notifications@github.com wrote:

It did work, thank you very much.

— Reply to this email directly or view it on GitHub https://github.com/fidian/git-started/issues/18#issuecomment-56410937.