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

Refactor part of printErrorReport into method #597

Closed

Conversation

ravage84
Copy link

This reduces the N path complexity of the printErrorReport method and improves the readability.

This reduces the N path complexity of the printErrorReport method and improves the readability.
@gsherwood
Copy link
Member

I don't see a reason to break the code out like this and I actually think it makes it harder to follow. You obviously disagree, which is fine.

If you have a specific need for this in a project, please let me know so I can make the change and test it for you. Otherwise, I'm going to leave that code as is.

@ravage84
Copy link
Author

Well, I was about to propose two CLI switches for ignoring warnings and errors respectively for when running a CS check.
Like:

php scripts/phpcs folder --standard=PHPCS  --ignore-errors --ignore-warnings

The reason for this is this:
thephpleague/skeleton#41

Without such switches, people would need set the config settings before.
Which means they either need to kow about or it needs to be documented.

And with my refactoring of the code, it would be cleaner to be implemented instead of making the currrent method printErrorReport even longer.

@gsherwood
Copy link
Member

Ignoring warnings is a pretty core feature, so it has a dedicated option -n

You can also use --warning-severity=0 to hide them (-n is a shortcut for this). You can do the same thing with errors by using --error-severity=0.

@ravage84
Copy link
Author

The problem is not hiding them, but the exit code, which produces Composer (or any other calling program) to recognise the execution as failed.

@ravage84
Copy link
Author

But --runtime-set probably solves the problem. Thanks

@gsherwood
Copy link
Member

Yeah sorry, I was getting confused by the exit code and the actual reports.

@ravage84
Copy link
Author

Anyway, the point of this PR was mainly that I think that PHPCS could benefit from shorter methods.
Not only in this case but in other places (as much as I've seen through the code).

So please reject this as you like.

@gsherwood
Copy link
Member

Anyway, the point of this PR was mainly that I think that PHPCS could benefit from shorter methods.

I have to balance that with the number of function calls. When running for several minutes, the overhead of those call really adds up.

But I've begun refactoring things in the 3.0 branch if you want to poke around. Some stuff is better, some stuff is the same.

@gsherwood gsherwood closed this May 20, 2015
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.

None yet

2 participants