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

Change to use `where` for validateExecutableExists #68

Closed sirbrillig closed 1 year ago

sirbrillig commented 1 year ago

Since type may not exist in Windows, this changes the validateExecutableExists function to use where instead.

Hopefully this will fix https://github.com/sirbrillig/phpcs-changed/issues/66

shivapoudel commented 1 year ago

@sirbrillig I got some leisure time to track this PR. Here goes my scenario:

    public function validateExecutableExists(string $name, string $command): void {
        // exec(sprintf("type %s > /dev/null 2>&1", escapeshellarg($command)), $ignore, $returnVal);
        // exec(sprintf("where %s > /dev/null 2>&1", escapeshellarg($command)), $ignore, $returnVal);
        exec(sprintf("where %s > nul 2>&1", escapeshellarg($command)), $ignore, $returnVal);
        if ($returnVal != 0) {
            throw new \Exception("Cannot find executable for {$name}, currently set to '{$command}'.");
        }
    }

For Windows that nul seems like a typo earlier in our conversation hopefully opt-out the message of Cannot find executable. But still, I broke some rules for PHPCS to fail and this phpcs-changed just passed it which is strange, my friend :)

shivapoudel commented 1 year ago

With the above line changed. Rather than lint-staged I tried to execute this one but failed to state like this:

image

image

Again for PHPCS executable which is installed only from composer in devDependencies this exists if command is executed image

shivapoudel commented 1 year ago

@sirbrillig any updates on this?

sirbrillig commented 1 year ago

Sorry! I've had a really busy week and I haven't had a chance to look at this more fully. I will get to it though! Thank you for your research so far.

sirbrillig commented 1 year ago

For Windows that nul seems like a typo earlier in our conversation hopefully opt-out the message of Cannot find executable. But still, I broke some rules for PHPCS to fail and this phpcs-changed just passed it which is strange, my friend

https://stackoverflow.com/questions/313111/is-there-a-dev-null-on-windows suggests that on windows /dev/null is supposed to be nul so I may need to detect Windows and alter that here. Still, from your screenshots above it looks like it should work.

Are you saying that when you make those changes, phpcs-changed does not complain about the executable but also does not return any results? If so, try running it with --debug to see what it is doing.

Again for PHPCS executable which is installed only from composer in devDependencies this exists if command is executed

The error message path '$(git' does not exist suggests that something strange is going on. That's not an error message from phpcs-changed so it must be coming from somewhere else and it also looks oddly formed with that dollar-sign and open-parenthesis.

As for the error message about not being able to find phpcs, can you try maybe setting the path to it explicitly using the environment variable PHPCS? So something like export PHPCS=./vendor/bin/phpcs and then run phpcs-changed --git -s file.php.

shivapoudel commented 1 year ago

Here goes the result from your above saying: image

shivapoudel commented 1 year ago

Regarding the linted-stage integration with and without --debug all commit are passed image

image

Applying the --debug also don't provide detailed information and in main plugin I have corrupted the rules knowingly:

<?php
/**
 * Plugin Name: WPForms Repeater
 * Plugin URI: https://github.com/WPCanny/wpforms-repeater
 * Description: Repeater for WPForms provides users with flexibility of repeating any number of fields in the form from the frontend.
 * Version: 1.4.0-dev
 * Author: WPCanny
 * Author URI: https://wpcanny.com
 * Text Domain: wpforms-repeater
 * Domain Path: /languages/
 * Requires at least: 5.4
 * Requires PHP: 5.6.20
 *
 * @package WPForms\Repeater
 */

// Exit if access directly.
defined( 'ABSPATH' ) || exit;

// WPForms Repeater version.
if (!defined( 'WPFORMS_REPEATER_VERSION' ) ) {
    define( 'WPFORMS_REPEATER_VERSION',  '1.4.0' );
}
shivapoudel commented 1 year ago

@sirbrillig I hope this result will help you further:

image

sirbrillig commented 1 year ago

Ah! That last output does help. I'll continue this in #66

sirbrillig commented 1 year ago

Closing because the package needs a lot of refactoring to support Windows without WSL. I'll start work on it but it will require much more than a simple change like this.