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

Fixes CSS preprocessor #21

Merged
merged 1 commit into from
Sep 8, 2012
Merged

Fixes CSS preprocessor #21

merged 1 commit into from
Sep 8, 2012

Conversation

svperfecta
Copy link
Contributor

Hey @CHH - When playing with PIPE today I found I couldn't get CSS directives to work at all. (I'm on PHP 5.3.16 if it matters)

The culprit was a mistake in a regular expression that accounts for multi-line statements. I fixed that (small change) and found that the resulting header had a bunch of blank lines in it. I'm stripping them out too. Finally, I added a new unit test to cover the bug.

Cheers!
Brian

EOL;

$myFile = "out";
$fh = fopen($myFile, 'w') or die("can't open file");
Copy link
Owner

Choose a reason for hiding this comment

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

You should never call die in a unit test, because it causes PHPUnit for exit instantly, and tests that would be run afterwards don't get run. You also don't get any PHPUnit output and an exit code of 0, which is not very helpful for CI systems.

If you've to, then use return $this->fail("can't open file") instead. But I assume this is test code which was accidentally left in, so it would be better to simply remove it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks!! Still picking up some php. (I'm a rubyist). I've been really impressed by the tools that have sprung up in the last few years. It's definitely not the PHP I learned 15 years ago :)

@CHH CHH merged commit 557dd1a into CHH:master Sep 8, 2012
@CHH
Copy link
Owner

CHH commented Sep 8, 2012

Apart from a nitty gritty detail, this PR is fine 😄. Thanks for the great work!

@svperfecta
Copy link
Contributor Author

Agh, I meant to delete that debug output. I'll do so later.

@CHH
Copy link
Owner

CHH commented Sep 9, 2012

No problem, I've already done that 😄

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