-
Notifications
You must be signed in to change notification settings - Fork 24
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
Final changes #123
Final changes #123
Conversation
@@ -18,6 +20,15 @@ public static function new(string $url, int $width, int $height): self | |||
return new Icon($url, $width, $height); | |||
} | |||
|
|||
public function getArray(): array |
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'm guessing the getArray()
is just how the model is returned? I was wondering why this was named so generically.
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.
Correct. I need something that builds the potential array of properties before sending in through the various concerns for processing. I settled on getArray()
but am open to suggestions for a better way to name this.
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 this is fine, I could understand what it was doing by just looking at the function.
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 this looks good, I don't have access to the lti-library repo in code climate. I'm not sure if you just need to invite people. It looks like the upload ran successfully so I think it should be fine, I'm sure it would throw an error if the code coverage wasn't formatted like the reporter expected.
https://github.com/packbackbooks/lti-1-3-php-library/actions/runs/7268165657/job/19803484110
Summary of Changes
Make a few final changes to clean things up, make them consistent, and abstract a bit more. Also add code coverage reports. It adds a
Arrayable
andJsonStringable
concerns to reduce code duplication.Testing
composer test
before opening this PRcomposer lint-fix
before opening this PR