[ical] Add DateParser
#534
Conversation
| DateParser::class, | ||
| new DateParser() | ||
| ); | ||
| } |
There was a problem hiding this comment.
I think this test is better omitted
There was a problem hiding this comment.
I do test the constructor for a while now and it served me well, it even found some issues in other tests.
|
|
||
| $timeValue = $this->getMockBuilder( '\SMWTimeValue' ) | ||
| ->disableOriginalConstructor() | ||
| ->getMock(); |
There was a problem hiding this comment.
Pretty sure you can use createMock rather than this deprecated form
You can also use SMWTimeValue::class
There was a problem hiding this comment.
I know but the intent is clearer that constructor is disabled, and we don't count lines so it doesn't really matter.
There was a problem hiding this comment.
Eh... both the authors of PHPUnit and me beg to differ
| ->disableOriginalConstructor() | ||
| ->getMock(); | ||
|
|
||
| $timeValue->expects( $this->any() ) |
There was a problem hiding this comment.
SMWTimeValue is a value object right? Then why are you creating a mock? This adds a bunch of hidden coupling (method names as strings, plus their signatures...) which is best avoided if there is no good reason for it.
There was a problem hiding this comment.
SMWTimeValue is a value object right?
Sort of but using an a mock ensures that specific coupling to let's say a Title object, or the Store are not pulled in hereby avoids creating a possible DB connection, especially the Title object is nasty, and by using a mock I'm certain I have an encapsulate object that doesn't create any connection.
There was a problem hiding this comment.
Alright, sounds like SMWTimeValue is not a value object then.
| * @group semantic-result-formats | ||
| * | ||
| * @license GNU GPL v2+ | ||
| * @since 3.1 |
There was a problem hiding this comment.
This class is not package public so this is clutter IMO
There was a problem hiding this comment.
I'm not a fan of the "no comment" purist approach, so I do think some minimal style block should be retained.
There was a problem hiding this comment.
I was talking just about the @SInCE tag
| * | ||
| * @return string | ||
| */ | ||
| public function parseDate( TimeValue $dataValue, $isEnd = false ) { |
There was a problem hiding this comment.
public function parseDate( TimeValue $dataValue, bool $isEnd = false ): string {
There was a problem hiding this comment.
Doing strict typing, I know you like it but I have not come to terms on how to approach this without creating a piece here and there. Without a clear map of how to transform an entire "package" (it would refer here to the entire ical format and its depending classes) and not just for one class I'm a bit reluctant to just drop it like a breadcrumb.
This PR is made in reference to: #
This PR addresses or contains:
DataValueFactoryg::getInstanceThis PR includes: