-
Notifications
You must be signed in to change notification settings - Fork 0
Output Link to ticket in found address CSV #1
Conversation
The output CSV can now contain a ticket ID. Move CSV generation to output class. Delete CSVSourceDataReader - exported CSV has no ticket id and we're using the database now.
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.
This talk is really boring... so you can has some reviews :)
|
||
trait CSVFormatter { | ||
|
||
protected function formatAsCSV( array $row ): string { |
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.
Why not private?
|
||
namespace WMDE\OtrsExtractAddress\UseCases\ExtractAddress; | ||
|
||
trait CSVFormatter { |
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.
Why trait and not composition? If you don't want to inject the thing I'd still make a class and just instantiate it where it is used.
if ( $v ) { | ||
return '"'. str_replace( '"', '""', $v ) . '"'; | ||
} else { | ||
return ''; |
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.
Oh noez! an if-esle
. You can use a guard followed by a return
|
||
private $outputStream; | ||
|
||
public function __construct( $outputStream ) { |
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.
Is it still not possible to type hint this in PHP 7.1?
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.
Nope, resources (DB resource, file handler resource), can't be type hinted yet. But I made another commit where I replace the resource with SplFileObject
.
Convert trait to class and use that class internally. Remove superfluous `else`.
+0.82 |
Resources can't be type hinted yet in PHP (at least until 7.1), see http://stackoverflow.com/questions/40637939/typehinting-for-filehandle-as-argument-in-function So we use SplFileObject instead.
$this->outputStream = $outputStream; | ||
$this->csvFormatter = new CSVFormatter(); | ||
} | ||
|
||
public function writeRow( SourceData $data, string $errorMsg ) { | ||
// TODO add option to skip "not found" messages and export only other messages | ||
fwrite( $this->outputStream, $this->csvFormatter->formatAsCSV( [ | ||
$this->outputStream->fwrite( $this->csvFormatter->formatAsCSV( [ |
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.
cool... no more global call
I don't see any obvious issues but cannot +2 without understanding the tasks and tools involved here more. Presumably @KaiNissen is better up to speed? |
The output CSV can now contain a ticket ID.
Move CSV generation to output class.
Delete CSVSourceDataReader - exported CSV has no ticket id and we're
using the database now.