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

Verify the installed_paths after save #97

Merged
merged 1 commit into from Jan 19, 2020

Conversation

jrfnl
Copy link
Member

@jrfnl jrfnl commented Jan 17, 2020

Proposed Changes

As most problems reported are to do with paths not being set correctly, let's verify success within the plugin, independently of PHPCS.

To that end, a new function verifySaveSuccess() has been added.
This function takes the paths which were to be saved and compares them to the paths which are actually set in PHPCS after the save to determine whether or not the plugin was successful and changes the exit code if this is not the case.

Open questions:

  • Should additional debug output be generated by the verification function when verbose mode is turned on ? Something like Expected paths to be set to: %s, found: %s.
    This could also come in handy to verify that all paths which should be set were found by the plugin.

At this moment I don't have a test case for this new functionality, however, I imagine it will come in useful with future bug reports.

src/Plugin.php Show resolved Hide resolved
@Potherca
Copy link
Member

Should additional debug output be generated by the verification function when verbose mode is turned on?

I know that goes against the principle of only reporting errors in output, but as this is in verbose mode, I would say Yes, definitely.

I imagine it will come in useful with future bug reports.

Fully agree. Should make life easier for all.

@jrfnl
Copy link
Member Author

jrfnl commented Jan 18, 2020

Oh dear ... while debugging why the build was failing, I've discovered a pretty interesting bug, which could well be the cause of some reported mystery issues. Win!

I'll work on a fix and send it in as a separate PR which will need to be merged before this PR as this PR will be dependent on it.

@Potherca
Copy link
Member

PR #98 has been merged.

@jrfnl
Copy link
Member Author

jrfnl commented Jan 19, 2020

Thanks, I'll update this PR in a moment and will make it mergable if the build then passes.

As most problems reported are to do with paths not being set correctly, let's verify success within the plugin, independently of PHPCS.

To that end, a new function `verifySaveSuccess()` has been added.
This function takes the paths which were to be saved and compares then to the paths which are actually set in PHPCS after the save to determine whether or not the plugin was successful and changes the exit code if this is not the case.

If there was a mismatch in paths and `verbose` mode is turned on, an information message will be displayed to help debugging.

At this moment I don't have a test case for this new functionality, however, I imagine it will come in useful with future bug reports.
@jrfnl jrfnl force-pushed the feature/add-save-success-verification branch from 1ca8c0d to 5277f67 Compare January 19, 2020 15:24
@jrfnl jrfnl marked this pull request as ready for review January 19, 2020 15:24
@jrfnl
Copy link
Member Author

jrfnl commented Jan 19, 2020

Marking this ready for review. I've updated the PR for:

  • code layout
  • verbose output on failure

@Potherca Potherca merged commit 6589501 into master Jan 19, 2020
@jrfnl jrfnl deleted the feature/add-save-success-verification branch January 19, 2020 15:38
@jrfnl
Copy link
Member Author

jrfnl commented Jan 19, 2020

Just now thinking: this should probably also run cleanInstalledPaths() to safeguard against the comparison failing for Windows vs Linux slashes. I'll send in a new PR to add that.

@jrfnl
Copy link
Member Author

jrfnl commented Jan 19, 2020

Oh second thought: no, as $this->installedPaths is only updated when the path doesn't exist, cleanInstalledPaths() doesn't update for slash differences.

Hmm.. let's see how we fare for now. If needs be, this can be revisited later.

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

Successfully merging this pull request may close these issues.

None yet

2 participants