search

JakubOnderka / PHP-Parallel-Lint

This tool check syntax of PHP files faster than serial check with fancier output.
Other
647 stars 62 forks source link

Optional Git blame #43

Closed VasekPurchart closed 9 years ago

VasekPurchart commented 9 years ago

I stumbled upon the new Git blame functionality introduced in 9930d122439c87275e49fc6550846e55baf46ca1 when updating all my projects to the 0.8 version finally and I noticed (correct me if I am wrong) that there is no straightforward way, to disable this functionality - if it will run is based only on the fact if there is a Git executable.

Using git blame is not really helpful in a lot of situations (depends on the workflow of the project) blame might not be a good tool for the job at all. And what's more - there are situations, when the file is not even committed to Git yet, so it is outright misleading.

The only way to disable the functionality is to set the --git cmd parameter to an non-existent executable. But I consider this approach really hacky and prone to error (no way to ensure there wont be such an executable on different systems, it's relying on current implementation detail, etc.),

I would prefer if there was a way to disable this by a cmd parameter (or be disabled by default and only optionally enabled?).

# disable blame functionality
./parallel-lint --no-blame . 

# or if it would be disabled by default
./parallel-lint --blame .

Please let me know, that do you think, I can try to prepare a PR, when/if we agree on one of the variants.

JakubOnderka commented 9 years ago

I think the better solution is parameter for optional disabling blame functionality. Or do you think, the blame is counterproductive in most cases?

VasekPurchart commented 9 years ago

As I wrote before - it is heavily dependent on the workflow, but for me the possibility that it would give you the false results without notice is enough not to use it.

Also it might slow down the operation, which is definitely not desirable, especially if might be worthless (such as in the case when the error is not yet commited). This problem I guess would be bigger on Windows, where some Git operations are much slower than on Linux.

And if there was something wrong or unexpected with the Git environment (might be a modified alias or modified output or something) which could cause strange (for the observer unrelated errors) - when I tried this version, some people even didn't notice the output of the blame.

Since it is not an essential feature which can cause some confusion I would vote for by default disabled, optionally enabled.

VasekPurchart commented 9 years ago

@JakubOnderka ping :) can you please look at this? Thx.

VasekPurchart commented 9 years ago

@JakubOnderka ping, another try :)

JakubOnderka commented 9 years ago

Sorry for later response. PhpParallelLint uses 'porcelain' Git output for machine consumption, so I think is compatible with other clients. And for uncommitted files can be added warning message or don't use blame for these files.

But personally I am not using this functionality, so if you think it is better disable it by default, you can send me pull request and I merge it.

VasekPurchart commented 9 years ago

But personally I am not using this functionality, so if you think it is better disable it by default, you can send me pull request and I merge it.

I am not using it as well :)

PR is ready: #45 .