Add gradesReleased to LtiLineItem #106
Conversation
src/LtiLineitem.php
Outdated
| return $this->grades_released; | ||
| } | ||
|
|
||
| public function setGradesReleased($value) |
There was a problem hiding this comment.
Is it possible to add typing to this variable? I looked at the spec, and it doesn't offer any indication as to what type(s) are allowed here. Is this a boolean, string, timestamp, black hole, something else?
Edit: I found some examples in the spec doc, and it looks like they're using bools.
| public function setGradesReleased($value) | |
| public function setGradesReleased(bool $value) |
There was a problem hiding this comment.
Its not called out in the spec, but the examples on the ags spec show it as a boolean. It would have to be parsed as such by us here. @JonathanGawrych is with me at GoReact and we noticed the gradesReleased property was added this january to the spec, so we'd like to just get ahead of that and build in support now.
There was a problem hiding this comment.
I was following the pattern of the other properties, not adding type information. Indeed the tests even pass the string 'expected' to every property, even numbers like getScoreMaximium. I added it without types because I wasn't sure which version of php was supported. It's not in the composer.json
I think it would be good if we did a seperate MR to add typing information, what do you think?. I see the tests are running 7.4. Something like activityProgress would be good as a string enum, as the only values are Initialized, Started, InProgress, Submitted, or Completed. Are there any plans to move to PHP >=8.1?
There was a problem hiding this comment.
Yeah, I've got some work to do first, but my goal is to set the minimum to >=8.1 and strictly type the entire package (to the extent that the LTI spec allows, at least). Either way, let's type this. I want to default to typing all new functions.
There was a problem hiding this comment.
Sorry for the delay. I've added typing information. Thanks!
JonathanGawrych
force-pushed
the
jag/add-grades-released
branch
from
ce6fa66
to
217bd70
Compare
src/LtiLineitem.php
Outdated
| return $this->grades_released; | ||
| } | ||
|
|
||
| public function setGradesReleased(?bool $value): static |
There was a problem hiding this comment.
| public function setGradesReleased(?bool $value): static | |
| public function setGradesReleased(?bool $value): self |
PHP 7.4 strikes again!
There was a problem hiding this comment.
Ah! I pushed an update to fix this.
JonathanGawrych
force-pushed
the
jag/add-grades-released
branch
from
217bd70
to
a1312a7
Compare
Summary of Changes
"gradesReleased" is a new feature of the Assignment and Grade Services 2.0 as of January 24th, 2023. This just adds the property to the line item model so that it may be retreived/set
Testing
composer testbefore opening this PRcomposer lint-fixbefore opening this PR