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

Adapt run.php to the new phpcs3 API #90

Merged
merged 1 commit into from
Oct 28, 2020
Merged

Conversation

sarjona
Copy link
Member

@sarjona sarjona commented Oct 23, 2020

The run.php file has been reviewed and adapted to use the new runner class.

Some changes have been done to this runner class to add some of the config settings and also to get the interactive mode working ($this->processFile($file) is the method doing the magic and the exception has been added too to exit when 'q' key is pressed).
The following code has been removed to avoid to repeat some information:
$reporter->cacheFileReport($file, $this->config);

The run.php file can be tested using the following commands:

  • php run.php -h : Should display the help information
  • php run.php local: Should run the codechecker over the moodle/local folder. Currently, it should return a couple of warnings in locallib.php file
  • php run.php -i local: Should run the codechecker using the interactive mode (so it will stop for every file having any error/warning).

@sarjona
Copy link
Member Author

sarjona commented Oct 27, 2020

The branch has been rebased to the latest 3.5.8 PHPCS3.

@vmdef vmdef assigned vmdef and unassigned vmdef Oct 28, 2020
@vmdef vmdef self-requested a review October 28, 2020 08:06
run.php Show resolved Hide resolved
run.php Show resolved Hide resolved
@vmdef
Copy link
Contributor

vmdef commented Oct 28, 2020

Hi Sara!
Nice work! Thanks for work on this patch. Just some comments for your consideration, although most of them are not part of your changes, I think it worth to fix them now:

  • Rebase onto current phpcs3 to include Fix comment in .travis.yml #93 and remove testing on 31 and 32 branches that fail.
  • classes/runner.php
    • line 75, typo in runned -> runner
    • line 60, the property to specify an output file is reportFile.
      After another read, I realized I was wrong. I interpreted the name of the method, set_reportfile(), literally
  • run.php
    • line 27, we can change dirname() to DIR
    • as we have the methods defined in \local_codechecker\runner, we could add las opciones para esconderse the warnings and specify an output file

Copy link
Member Author

@sarjona sarjona left a comment

Choose a reason for hiding this comment

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

Hi Víctor!

Thanks for reviewing the patch and raising these interesting comments :-)

  • I've rebased the patch to the latest.
  • classes/runner.php
    • fixed typo (well spotted!)
    • I haven't changed anything in this method and I think it's OK as it is :-)
  • run.php

So the patch is ready again to get your eyes over it ;-)

run.php Show resolved Hide resolved
run.php Show resolved Hide resolved
Copy link
Contributor

@vmdef vmdef left a comment

Choose a reason for hiding this comment

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

Thanks, Sara. Your proposal is sensible. Let's go as it is and we will address these questions in other issues.

@sarjona
Copy link
Member Author

sarjona commented Oct 28, 2020

A separate issue has been created to see if includes in index.php and run.php can be removed: #94

@vmdef vmdef merged commit e805a09 into moodlehq:phpcs3 Oct 28, 2020
@stronk7
Copy link
Member

stronk7 commented Nov 2, 2020

yay * 2!

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

Successfully merging this pull request may close these issues.

3 participants