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

Add support for home dir as tilde #354

Closed
wants to merge 1 commit into from

Conversation

antonioribeiro
Copy link

This is a fix for #353, so we can call it this way:

phpcs -v --standard=~/.composer/vendor/pragmarx/laravelcs/Standards/Laravel /path/to/folder/or/file

@gsherwood
Copy link
Member

I don't think home directory expansion is something PHP_CodeSniffer should be doing. It's something the shell should be doing, so I'm not going to merge this.

@gsherwood gsherwood closed this Dec 9, 2014
@gsherwood gsherwood reopened this Dec 10, 2014
@gsherwood
Copy link
Member

Actually, I think I have changed my mind. I'll look into this a bit more.

Sorry for closing without thinking it all the way through.

@antonioribeiro
Copy link
Author

It's okay!

It's your project and it's hard for someone far from it say what's best. Unfortunately PHP has a lot of flaws itself and I think the tilde expansion should be also handled by realpath().

But I also understand that this is, usually, an environment matter, so, yes it's complicated.

But for Linux users using the tilde kind of normal, so you may have some people complaiting about it, then I was only trying to make easier the life of those that didn't bumped on that problem themselves yet, because, to me now, it makes not much difference to use ~ or $HOME.

The problem is that you should at least inform the user that "the usage of tilde is not supported", becase PHP_CodeSniffer is currently only saying "there is no such standard", and that can cause a lot of confusion, since the standard folder exists, it's only the folder reference that's not good.

The problem was first raised on a new Standard I'm working, here you can see that it took some time to understand what was really the problem: antonioribeiro/laravelcs#2.

Cheers!

@gsherwood
Copy link
Member

I've had a look at the code and I've done things a little differently to ensure the concept of installed standards remains consistent. So I'll close this PR and continue this on issue #353.

@gsherwood gsherwood closed this Dec 10, 2014
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