Skip to content

Add watch option to lint commands #26120

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 1 commit into from
Closed

Add watch option to lint commands #26120

wants to merge 1 commit into from

Conversation

pierredup
Copy link
Contributor

@pierredup pierredup commented Feb 9, 2018 •
edited
Loading

Q A
Branch? master
Bug fix? no
New feature? yes
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets N/A
License MIT
Doc PR TDB

This PR actually has 2 parts:

  • Add a watch method to the Filesystem component to monitor the filesystem for changes, and executing a callback function whenever a change is detected

The watch method can take any parameter and uses a resource locator to transform a path to a resource. The resource can then be monitored for any changes. When a directory is monitored, you can get changed events when a file in the directory changes, when a new file is created or when an existing file is deleted. It also tried to make usage of Inotify is the extension is installed.

Example usage:

$fs = new Filesystem();

$fs->watch('/path/to/some/directory', function ($file, $event) {
    if ($event === FileChangeEvent::FILE_CREATED) {
        echo $file.' was created!';
    }
});
  • Add a watch option to all the lint commands

When the watch option is used, the process will monitor the filesystem for any changes. When a change is detected, the changed file will be re-validated and the output printed to the console.

Simperfit
Simperfit reviewed Feb 9, 2018
View reviewed changes
src/Symfony/Component/Filesystem/Tests/Watcher/Resource/ArrayResourceTest.php

declare(strict_types=1);

/*
Copy link
Contributor

Choose a reason for hiding this comment

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

this should be changed

Simperfit
Simperfit reviewed Feb 9, 2018
View reviewed changes
src/Symfony/Component/Filesystem/Tests/Watcher/Resource/ArrayResourceTest.php Outdated
@@ -0,0 +1,35 @@
<?php

declare(strict_types=1);
Copy link
Contributor

Choose a reason for hiding this comment

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

we don't use strict_types atm in the codebase ATM

Simperfit
Simperfit reviewed Feb 9, 2018
View reviewed changes
src/Symfony/Bridge/Twig/Command/LintCommand.php Outdated
throw new \RuntimeException('The symfony/filesystem package is required in order to watch files for changes.');
}

$io->note('Watching files for changes');
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe a final dot or a ... ?

@sroze
Copy link
Contributor

sroze commented Feb 9, 2018

It might be worth opening an RFC issue to identify whether this makes sense to include this in Symfony. I would argue it is but still, it needs to be discussed :)

@pierredup pierredup force-pushed the lint-watch-files branch 2 times, most recently from 3c2efa3 to 9961be3 Compare February 9, 2018 19:11
@nicolas-grekas nicolas-grekas added this to the 4.1 milestone Feb 9, 2018
@Koc
Copy link
Contributor

Koc commented Feb 11, 2018

What about splitting PR to two parts: first for adding watching for filesystem, second for lint commands?

nicolas-grekas
nicolas-grekas reviewed Feb 14, 2018
View reviewed changes
src/Symfony/Component/Translation/CHANGELOG.md Outdated
@@ -6,6 +6,7 @@ CHANGELOG

* The `FileDumper::setBackup()` method is deprecated and will be removed in 5.0.
* The `TranslationWriter::disableBackup()` method is deprecated and will be removed in 5.0.
* Added `--watch` option to the `lint:twig` command to watch the filesystem for changes and re-lint any changed files
Copy link
Member

Choose a reason for hiding this comment

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

lint:xliff

@nicolas-grekas
Copy link
Member

nicolas-grekas commented Feb 19, 2018 •
edited
Loading

Would it make sense to have a single new lint command that would look at extensions to decide the format of the file, and would be the only one having --watch, to make things simpler?

@ostrolucky
Copy link
Contributor

I'm for having single command, but what's the best location to have it in? FileSystem component?

ostrolucky
ostrolucky suggested changes Mar 20, 2018
View reviewed changes
Copy link
Contributor

@ostrolucky ostrolucky left a comment

Choose a reason for hiding this comment

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

composer.json files need to be changed

@pierredup
Copy link
Contributor Author

composer.json files need to be changed

@ostrolucky What must be changed?

@ostrolucky
Copy link
Contributor

ostrolucky commented Mar 20, 2018 •
edited
Loading

symfony/filesystem < 4.1 conflict section should be added. And maybe add it to suggest section

@nicolas-grekas nicolas-grekas modified the milestones: 4.1, next Apr 20, 2018
@pierredup
Copy link
Contributor Author

Going through this PR again, I don't think adding a watch option for lint commands is a good use case. But the watch method in the Filesystem component is still very useful, so I rather extracted that to a separate PR in #31462

@pierredup pierredup closed this May 10, 2019
@nicolas-grekas nicolas-grekas modified the milestones: next, 4.4 Oct 27, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nicolas-grekas nicolas-grekas nicolas-grekas left review comments

@Simperfit Simperfit Simperfit left review comments

@ostrolucky ostrolucky ostrolucky requested changes

Assignees
No one assigned
Labels
Feature Status: Needs Work
Projects
None yet
Milestone
4.4
Development

Successfully merging this pull request may close these issues.
7 participants
@pierredup @sroze @Koc @nicolas-grekas @ostrolucky @Simperfit @carsonbot