Fix Markdown screenshot URL + Add headers to client call + Some renaming + Add metadata to files + Units in paper size #41
Conversation
| $splitAndParseStringWithUnit = static function (mixed $raw, callable $callback): void { | ||
| [$value, $unit] = sscanf((string) $raw, '%d%s') ?? throw new \InvalidArgumentException(sprintf('Unexpected value "%s", expected format is "%%d%%s"', $raw)); | ||
|
|
||
| $callback((float) $value, Unit::tryFrom((string) $unit) ?? Unit::Inches); |
There was a problem hiding this comment.
Maybe we could emit a log if the unit is invalid ?
There was a problem hiding this comment.
Yes. Will do in another PR where I will add several logs like this one.
| @@ -377,7 +380,7 @@ private function addConfiguration(string $configurationName, mixed $value): void | |||
| 'width' => $this->width($value), | |||
| 'height' => $this->height($value), | |||
| 'clip' => $this->clip($value), | |||
| 'format' => $this->format($value), | |||
| 'format' => $this->format(ScreenshotFormat::from($value)), | |||
There was a problem hiding this comment.
Is the error handled if the format is not valid ?
There was a problem hiding this comment.
nope it simply fails (in this case it will fail on clear-cache)
There was a problem hiding this comment.
OK. Maybe we should throw a InvalidBuilderConfiguration exception instead to make debug easier.
There was a problem hiding this comment.
Not needed. The Configuration class already handles this. I do not see a proper usecase for this exception at the moment.
| self::assertSame(['marginTop' => '1in'], $multipartFormData[3]); | ||
| self::assertSame(['marginBottom' => '1in'], $multipartFormData[4]); | ||
| self::assertSame(['marginLeft' => '1in'], $multipartFormData[5]); | ||
| self::assertSame(['marginRight' => '1in'], $multipartFormData[6]); |
There was a problem hiding this comment.
Since, in is the default one, we should add a test with an other one.
There was a problem hiding this comment.
agree. Several tests needs to be written actually
There was a problem hiding this comment.
I will create a dedicated PR after #40 is merged to rework some of the tests to better reflect all usecases.
Resolves #38
Resolves #45