[AI Store] Remove webmozart/assert library
#86
Conversation
| public Metadata $metadata = new Metadata(), | ||
| ) { | ||
| Assert::stringNotEmpty(trim($this->content)); | ||
| '' !== trim($this->content) || throw new \InvalidArgumentException('The content shall not be an empty string'); |
There was a problem hiding this comment.
Let's be more explicit and do
if ('' === trim($this->content)) {
throw new \InvalidArgumentException('The content shall not be an empty string');
}
There was a problem hiding this comment.
Hi @Nyholm,
You're absolutely right the if notation is more explicit. However in this case I opted for the shorter expression to maintain consistency across classes that perform many inline validations, such as the With class.
My goal was to keep the constructor concise and uniform, even if slightly less explicit.
That said, if you feel strongly about it, I will witch to the if version for clarity.
|
yes, agree as well, but for the Platform component you'd need to replace more code - or keep it for now and only take action on store |
|
Yes indeed. I will continue this PR when I get back home on Thursday. |
chr-hertel
added
Store
Spomky
force-pushed
the
deps/no-assert
branch
4 times, most recently
from
c774eb2 to
750d75c
Compare
| !str_starts_with($this->baseUrl, 'http://') || throw new \InvalidArgumentException('The base URL must not contain the protocol.'); | ||
| !str_starts_with($this->baseUrl, 'https://') || throw new \InvalidArgumentException('The base URL must not contain the protocol.'); | ||
| '' !== $deployment || throw new \InvalidArgumentException('The deployment must not be empty.'); | ||
| '' !== $apiVersion || throw new \InvalidArgumentException('The API version must not be empty.'); | ||
| '' !== $apiKey || throw new \InvalidArgumentException('The API key must not be empty.'); |
There was a problem hiding this comment.
Let's us the component specific exception here please: Symfony\AI\Platform\Exception\InvalidArgumentException
There was a problem hiding this comment.
same for the other places as well
There was a problem hiding this comment.
Indeed I had missed this class.
I have updated it wherever necessary.
OskarStark
changed the title
webmozart/assert library
|
I will prepare a PR to add unit tests for the code parts using the lib. So we can merge my PR first, then rebase and check the CI 👍 |
|
Here you go: |
|
Merged Oskar's tests, rebase please :) |
… input validation
|
Thanks. Rebased, squashed and tests are ok. |
|
From a design point of view my understanding was that we want exceptions clustered per package - rather a best practice than tho. |
|
Yes per package, imagine you are just using one package, the exception will not be available. |
|
Thank you @Spomky. |
|
You are welcome. |
4 participants
Hi,
I am not sure w need the
webmozart/assertdependency for just one verification.Let me squash this PR before any action (I am not with my usual setup at the moment).
Regards.