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

Final changes #123

Merged
merged 33 commits into from
Dec 21, 2023
Merged

Final changes #123

merged 33 commits into from
Dec 21, 2023

Conversation

dbhynds
Copy link
Member

@dbhynds dbhynds commented Dec 18, 2023

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 and JsonStringable concerns to reduce code duplication.

Testing

  • I have added automated tests for my changes
  • I ran composer test before opening this PR
  • I ran composer lint-fix before opening this PR

@dbhynds dbhynds changed the base branch from v6-phpstan to v6-deep-link December 19, 2023 17:29
@@ -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
Copy link

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.

Copy link
Member Author

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.

Copy link

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.

Copy link

@cophaso cophaso left a 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

@dbhynds dbhynds requested a review from cophaso December 20, 2023 21:48
@dbhynds dbhynds mentioned this pull request Dec 21, 2023
4 tasks
Base automatically changed from v6-deep-link to v6.0-beta December 21, 2023 19:22
@dbhynds dbhynds merged commit 344a704 into v6.0-beta Dec 21, 2023
4 checks passed
@dbhynds dbhynds deleted the final-changes branch December 21, 2023 19:22
@dbhynds dbhynds mentioned this pull request Dec 21, 2023
3 tasks
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