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

[ical] Add DateParser #534

Merged
merged 1 commit into from Aug 24, 2019
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
64 changes: 64 additions & 0 deletions src/iCalendar/DateParser.php
@@ -0,0 +1,64 @@
<?php

namespace SRF\iCalendar;

use SMWDataValueFactory as DataValueFactory;
use SMWTimeValue as TimeValue;

/**
* @license GNU GPL v2+
* @since 3.1
*/
class DateParser {

/**
* Extract a date string formatted for iCalendar from a SMWTimeValue object.
*
* @since 3.1
*
* @param TimeValue $dataValue
* @param boolean $isEnd
*
* @return string
*/
public function parseDate( TimeValue $dataValue, $isEnd = false ) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

public function parseDate( TimeValue $dataValue, bool $isEnd = false ): string {

Copy link
Contributor Author

@mwjames mwjames Aug 24, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

$year = $dataValue->getYear();

// ISO range is limited to four digits
if ( ( $year > 9999 ) || ( $year < -9998 ) ) {
return '';
}

$year = number_format( $year, 0, '.', '' );
$time = str_replace( ':', '', $dataValue->getTimeString( false ) );

// increment by one day, compute date to cover leap years etc.
if ( ( $time == false ) && ( $isEnd ) ) {
$dataValue = DataValueFactory::getInstance()->newDataValueByType(
'_dat',
$dataValue->getWikiValue() . 'T00:00:00-24:00'
);
}

$month = $dataValue->getMonth();

if ( strlen( $month ) == 1 ) {
$month = '0' . $month;
}

$day = $dataValue->getDay();

if ( strlen( $day ) == 1 ) {
$day = '0' . $day;
}

$result = $year . $month . $day;

if ( $time != false ) {
$result .= "T$time";
}

return $result;
}

}
52 changes: 8 additions & 44 deletions src/iCalendar/iCalendarFileExportPrinter.php
Expand Up @@ -41,6 +41,11 @@ class iCalendarFileExportPrinter extends FileExportPrinter {
*/
private $icalTimezoneFormatter;

/**
* @var DateParser
*/
private $dateParser;

/**
* @see ResultPrinter::getName
*
Expand Down Expand Up @@ -149,6 +154,8 @@ protected function getResultText( QueryResult $res, $outputMode ) {
*/
private function getIcal( QueryResult $res ) {

$this->dateParser = new DateParser();

$this->icalTimezoneFormatter = new IcalTimezoneFormatter();

$this->icalTimezoneFormatter->setLocalTimezones(
Expand Down Expand Up @@ -255,7 +262,7 @@ private function getIcalForItem( array $row ) {
if ( $dataValue === false ) {
unset( $params[$label] );
} else {
$params[$label] = $this->parsedate( $dataValue, $label == 'end' );
$params[$label] = $this->dateParser->parseDate( $dataValue, $label == 'end' );

$timestamp = strtotime( $params[$label] );
if ( $from === null || $timestamp < $from ) {
Expand Down Expand Up @@ -313,49 +320,6 @@ private function getIcalForItem( array $row ) {
return $result;
}

/**
* Extract a date string formatted for iCalendar from a SMWTimeValue object.
*/
private function parsedate( TimeValue $dv, $isend = false ) {
$year = $dv->getYear();

// ISO range is limited to four digits
if ( ( $year > 9999 ) || ( $year < -9998 ) ) {
return '';
}

$year = number_format( $year, 0, '.', '' );
$time = str_replace( ':', '', $dv->getTimeString( false ) );

// increment by one day, compute date to cover leap years etc.
if ( ( $time == false ) && ( $isend ) ) {
$dv = DataValueFactoryg::getInstance()->newDataValueByType(
'_dat',
$dv->getWikiValue() . 'T00:00:00-24:00'
);
}

$month = $dv->getMonth();

if ( strlen( $month ) == 1 ) {
$month = '0' . $month;
}

$day = $dv->getDay();

if ( strlen( $day ) == 1 ) {
$day = '0' . $day;
}

$result = $year . $month . $day;

if ( $time != false ) {
$result .= "T$time";
}

return $result;
}

/**
* Implements esaping of special characters for iCalendar properties of type
* TEXT. This is defined in RFC2445 Section 4.3.11.
Expand Down
74 changes: 74 additions & 0 deletions tests/phpunit/Unit/iCalendar/DateParserTest.php
@@ -0,0 +1,74 @@
<?php

namespace SRF\Tests\iCalendar;

use SRF\iCalendar\DateParser;

/**
* @covers \SRF\iCalendar\DateParser
* @group semantic-result-formats
*
* @license GNU GPL v2+
* @since 3.1
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This class is not package public so this is clutter IMO

Copy link
Contributor Author

@mwjames mwjames Aug 24, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not a fan of the "no comment" purist approach, so I do think some minimal style block should be retained.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was talking just about the @SInCE tag

*
* @author mwjames
*/
class DateParserTest extends \PHPUnit_Framework_TestCase {

public function testCanConstruct() {

$this->assertInstanceOf(
DateParser::class,
new DateParser()
);
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this test is better omitted

Copy link
Contributor Author

@mwjames mwjames Aug 24, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I do test the constructor for a while now and it served me well, it even found some issues in other tests.


public function testParseDate_Year() {

$timeValue = $this->getMockBuilder( '\SMWTimeValue' )
->disableOriginalConstructor()
->getMock();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pretty sure you can use createMock rather than this deprecated form

You can also use SMWTimeValue::class

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I know but the intent is clearer that constructor is disabled, and we don't count lines so it doesn't really matter.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Eh... both the authors of PHPUnit and me beg to differ


$timeValue->expects( $this->any() )
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Alright, sounds like SMWTimeValue is not a value object then.

->method( 'getYear' )
->will( $this->returnValue( 2000 ) );

$instance = new DateParser();

$this->assertSame(
'20000101',
$instance->parseDate( $timeValue, true )
);
}

public function testParseDate_Year_Month_Day_Time() {

$timeValue = $this->getMockBuilder( '\SMWTimeValue' )
->disableOriginalConstructor()
->getMock();

$timeValue->expects( $this->any() )
->method( 'getYear' )
->will( $this->returnValue( 2000 ) );

$timeValue->expects( $this->any() )
->method( 'getMonth' )
->will( $this->returnValue( 12 ) );

$timeValue->expects( $this->any() )
->method( 'getDay' )
->will( $this->returnValue( 12 ) );

$timeValue->expects( $this->any() )
->method( 'getTimeString' )
->will( $this->returnValue( '12:01:01' ) );

$instance = new DateParser();

$this->assertSame(
'20001212T120101',
$instance->parseDate( $timeValue, true )
);
}

}