Skip to content
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

error when no files will be processed #4326

Merged
merged 2 commits into from
Jun 26, 2023
Merged

Conversation

staabm
Copy link
Contributor

@staabm staabm commented Jun 23, 2023

before this PR:

$ php bin/rector process some/unkown/file.php --dry-run -vvv 
                                                                                                                        
 [OK] Rector is done!                                                                                                   

$ echo $? 
0 # non-error exit code

after this PR:

$ php bin/rector process some/unkown/file.php --dry-run -vvv 
                                                                                                                        
 [ERROR] The given paths do not match any files                                                                         

$ echo $?
1 # error exit code
                                                               

refs rectorphp/rector#8003 (comment)

@staabm staabm marked this pull request as ready for review June 23, 2023 11:50
@staabm
Copy link
Contributor Author

staabm commented Jun 23, 2023

@7thstorm could you verify this matches your expectations?

@samsonasik
Copy link
Member

@staabm if no path specified in command arg, is it still working?

@staabm
Copy link
Contributor Author

staabm commented Jun 25, 2023

if no path specified in command arg, is it still working?

yes:

 php bin/rector.php
 2256/2256 [▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓] 100%

                                                                                                                        
 [OK] Rector is done!                                                                                                   

@staabm
Copy link
Contributor Author

staabm commented Jun 26, 2023

@samsonasik can this be merged?

@samsonasik
Copy link
Member

I will let @TomasVotruba decide on it

@@ -72,6 +72,10 @@ protected function execute(InputInterface $input, OutputInterface $output): int

// 1. add files and directories to static locator
$this->dynamicSourceLocatorDecorator->addPaths($paths);
if ($this->dynamicSourceLocatorDecorator->isPathsEmpty()) {
$this->rectorOutputStyle->error("The given paths do not match any files");
return ExitCode::FAILURE;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

while probably rare usecase, I think show warning only is better than return error, to avoid a use case that file already removed via other rector rule.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not sure how files which are deleted from rector itself can affect the configuration-paths?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

User may can create a custom FileProcessor, which may delete file on specific use, then on parallel, it called on separate rules.

For DX perspective, I think stop and retry should not happen, and this also doesn't give information which part that empty.

@7thstorm
Copy link

7thstorm commented Jun 26, 2023

@7thstorm could you verify this matches your expectations?

sorry, I was out of town.
currently, if no path is specified in rector.php and no path is specified in command line, it acts as if everything is fine. the output is:

{
    "totals": {
        "changed_files": 0,
        "errors": 0
    }
}

I'm using Rector 0.17.1

how do I test new update?

But it seems the proposed change ""The given paths do not match any files" should be good enough

@staabm
Copy link
Contributor Author

staabm commented Jun 26, 2023

@7thstorm you could checkout this pull request and run rector from the project root with php bin/rector ...

@TomasVotruba
Copy link
Member

Let's ship this to main branch to test out the expectations.

@staabm Thank you for this DX improvement 👍

@TomasVotruba TomasVotruba merged commit 3ca5734 into rectorphp:main Jun 26, 2023
40 checks passed
@staabm staabm deleted the err-no-files branch June 26, 2023 19:10
@samsonasik
Copy link
Member

@staabm it seems cause error in e2e test in rector/rector, see https://github.com/rectorphp/rector/actions/runs/5382084725/jobs/9767016438#step:5:1

@samsonasik
Copy link
Member

@staabm The error seems because no path specified, it possibly that previous behaviour, when no path specified, it go to current directory as . , could you check it?

https://github.com/rectorphp/rector/blob/9da21b49dc4c5d276cc9c5ae75fa12bf592d1683/e2e/finalize/rector.php#L9

@staabm
Copy link
Contributor Author

staabm commented Jun 26, 2023

Will have a look a tomorrow

@samsonasik
Copy link
Member

The original behaviour is probably incorrect, as it prevoiusly shows:


Run ../../bin/rector process --dry-run --ansi
                                                                           
 [OK] Rector is done!                                                           

see https://github.com/rectorphp/rector/actions/runs/5381537795/jobs/9765738995#step:5:1

which if applied, it should show:

➜  finalize git:(main) ✗ php ../../bin/rector.php --dry-run
 2/2 [▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓] 100%
1 file with changes
===================

1) src/SomeClassWithoutChildren.php:3

    ---------- begin diff ----------
@@ @@

 namespace Rector\e2e;

-class SomeClassWithoutChildren
+final class SomeClassWithoutChildren
 {
 }
    ----------- end diff -----------

Applied rules:
 * FinalizeClassesWithoutChildrenRector

but I probably missing something, I will let you check ;)

@staabm
Copy link
Contributor Author

staabm commented Jun 27, 2023

but I probably missing something, I will let you check ;)

I think you are right, that the test is bogus

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants