rtCamp / blank-theme

Customized Blank theme based on Underscore and Foundation
25 stars 23 forks source link

PHPCS warnings and errors #20

Closed manishsongirkar closed 4 years ago

manishsongirkar commented 5 years ago

PHPCS error and warnings in inc/classes/class-base.php

Checked with WordPress coding standard.


FOUND 1 ERROR AND 3 WARNINGS AFFECTING 4 LINES
----------------------------------------------------------------------------------------------------------
  50 | ERROR   | Visibility must be declared on method "__destruct"
 149 | WARNING | trigger_error() found. Debug code should not normally be used in production.
 195 | WARNING | Method name "_add_hook" should not be prefixed with an underscore to indicate visibility
 216 | WARNING | trigger_error() found. Debug code should not normally be used in production.
----------------------------------------------------------------------------------------------------------```
DevikVekariya commented 4 years ago

@kiranpotphode please check the errors and let's add a GH action with phpcs checks here.

I'll help to add tokens to integrate the GH actions.

kiranpotphode commented 4 years ago

@DevikVekariya Added PR for the GH action for PHPCS checks. Please help with adding Secrets for the repo. https://github.com/rtCamp/blank-theme/pull/34/files

DevikVekariya commented 4 years ago

@kiranpotphode I just added the secrets VAULT_TOKEN and VAULT_ADDR to this repo

kiranpotphode commented 4 years ago

Thanks @DevikVekariya

PHPCS action executed successfully. You can approve and merge this PR https://github.com/rtCamp/blank-theme/pull/34.

Also https://github.com/rtCamp/blank-theme/issues/20#issue-390542056 File in which Errors are mentioned is no longer exist.

So you can merge the PR and close this issue.

DevikVekariya commented 4 years ago

PR looks good to me #34

Can we test it with one phpcs error, please?

kiranpotphode commented 4 years ago

Can we test it with one phpcs error, please?

I don't know if this will work unless code is merged in master or not. Can we merge PR in master first?

DevikVekariya commented 4 years ago

Yeah, we can test it in that way. Approved the PR please go ahead to merge

kiranpotphode commented 4 years ago

@DevikVekariya this is solved in #42 So we can close this issue.

DevikVekariya commented 4 years ago

Yeah. Thanks @kiranpotphode

Closing this now.