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

Only run orig file PHPCS linting when the new version has lint issues #51

Merged

Conversation

david-binda
Copy link
Collaborator

When there are no linting issues reported by PHPCS in the new file, there is no point in checking the orig verion, since we would not report any issues anyway.

This should speed up the phpcs-changed for files w/o lint issues.

props to @sirbrillig for the idea.

When there are no linting issues reported by PHPCS in the new file, there is no point in checking the orig verion, since we would not report any issues anyway.

This should speed up the phpcs-changed for files w/o lint issues.

props to @sirbrillig for the idea.
Do not set the `$newFilePhpcsOutput` variable in catch statement as it's not actually being used later in the code.

In this commit, the value of `$newFilePhpcsMessages` variable is being calculated from `PhpcsMessages::fromPhpcsJson` method with an empty string passed in as param, since it's how it's calculated for the `$oldFilePhpcsOutput`.
@david-binda david-binda marked this pull request as ready for review February 3, 2022 15:29
Copy link
Owner

@sirbrillig sirbrillig left a comment

Choose a reason for hiding this comment

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

This looks great! Nice job; I thought it would be harder to refactor for this behavior.

The only thing I'd like to do is have some tests to prove this behavior (mainly to guard against future regressions).

…ly to new behaviour

As we no longer scan orig file when there are no lint messages from new one, we should not register the `svn cat` command, but instead check that it was not called, which means that the new functionality works as expected.
@david-binda
Copy link
Collaborator Author

Yep, good call! I have added a new test for git workflow, and adjusted existing one for svn workflow. I made sure the test are properly failing w/o the logic change from this PR, and are passing when the change is applied.

@sirbrillig
Copy link
Owner

Thank you!

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.

2 participants