-
Notifications
You must be signed in to change notification settings - Fork 3.6k
Conversation
Fix indenting to make builds pass
@rudkjobing Very odd, all the other PRs are doing Travis builds, why not this one? I don't think it's anything to do with this PR or the code in it. |
…ument. -Changes from code review. -Rename randomBody method to randomHTMLBody Fix indenting
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have just noticed whilst using it for real, that it seems to sometimes return "textless" HTML, e.g.
<div id="36115"></div><div id="21890"><div class="nam"></div><div id="80145"></div><div class="ut"></div></div><div class="qui"></div>
I suspect this is not what we want, so I'm going to have a poke around and maybe submit a PR to your branch if I solve it.
I'll be playing around here...
https://github.com/BiffBangPow/Faker/tree/fake_html_body
Update: I think I've done it, but I need to dash off and be a drummer now, so will review it in the morning.
* | ||
* @return string | ||
*/ | ||
public function randomBodyFragments($maxDepth = 4, $maxWidth = 4) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could this be randomHTMLFragments
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have done this in my fork
@@ -20,10 +20,27 @@ public function testProvider() | |||
public function testRandomHtmlReturnsValidHTMLString(){ | |||
$faker = new Generator(); | |||
$faker->addProvider(new HtmlLorem($faker)); | |||
$node = $faker->randomHtml(6, 10); | |||
$node = $faker->randomHtml(5, 5); | |||
$dom = new \DOMDocument(); | |||
$error = $dom->loadHTML($node); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think $error
should be $isValidHtml
like in the test below.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done in my fork.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@rudkjobing I've almost got it never making HTML without any actual text in it. Still something in there does no text sometimes. I'm working on that.
Because the builds aren't playing ball with this PR, once I get that last bit sorted, I'll maybe try doing a PR back to fzaninotto/Faker and see if it does builds.
@@ -129,7 +159,8 @@ private function addRandomAttribute(\DOMElement $node) | |||
$node->setAttribute("class", $this->generator->word); | |||
break; | |||
case 2: | |||
$node->setAttribute("id", (string)$this->idGenerator->randomNumber(5)); | |||
$randomNumber = $this->generator->randomNumber(5); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This line want another space, or it the builds will fail on the code sniffing stage.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It was tabs not spaces. Done in my fork.
* | ||
* @return string | ||
*/ | ||
public function randomBodyFragments($maxDepth = 4, $maxWidth = 4) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have done this in my fork
@@ -20,10 +20,27 @@ public function testProvider() | |||
public function testRandomHtmlReturnsValidHTMLString(){ | |||
$faker = new Generator(); | |||
$faker->addProvider(new HtmlLorem($faker)); | |||
$node = $faker->randomHtml(6, 10); | |||
$node = $faker->randomHtml(5, 5); | |||
$dom = new \DOMDocument(); | |||
$error = $dom->loadHTML($node); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done in my fork.
@rudkjobing Latest update is, that I think I'm on top of it, but the builds are only working PHP 5.6+. I've just installed 5.3.29 with PHPBrew to see what that's about. |
Hi @rudkjobing I have a PR with passing builds here #1238. If you're happy to roll with that, please could you kill this PR to avoid confusion. Thanks, Will |
Thanks @rudkjobing I just corrected the link above to my PR. |
@WillGibson you are very welcome. Thanks for picking up my slack :-) |
No description provided.