Skip to content

W
WE.SpreadsheetImport
Commit c05951cd authored by Simon Gadient's avatar Simon Gadient
Browse files
Options

[RFT] Code improvements, comments, and tests 

Code improvements to increase performance. Additional tests and
comments.

refs KIME-4583
parent ed623bd8
Hide whitespace changes
Inline Side-by-side
Showing with 363 additions and 119 deletions
Classes/WE/SpreadsheetImport/Domain/Model/SpreadsheetImport.php
View file @ c05951cd
......@@ -166,7 +166,11 @@ class SpreadsheetImport {
* @return array
*/
public function getArguments() {
return unserialize($this->arguments);
$arguments = unserialize($this->arguments);
if (! is_array($arguments)) {
$arguments = array();
}
return $arguments;
}
/**
......
Classes/WE/SpreadsheetImport/SpreadsheetImportService.php
View file @ c05951cd
......@@ -12,22 +12,27 @@ namespace WE\SpreadsheetImport;
* */
use TYPO3\Flow\Annotations as Flow;
use TYPO3\Flow\Persistence\QueryInterface;
use TYPO3\Flow\Persistence\RepositoryInterface;
use WE\SpreadsheetImport\Annotations\Mapping;
use WE\SpreadsheetImport\Domain\Model\SpreadsheetImport;
/**
* Service to handle the column mapping and the import itself
*
* @Flow\Scope("singleton")
*/
class SpreadsheetImportService {
/**
* Domain object containing the configuration the service works with
*
* @var SpreadsheetImport
*/
protected $spreadsheetImport;
/**
* Class name of the domain model to be imported
*
* @var string
*/
protected $domain;
......@@ -72,33 +77,56 @@ class SpreadsheetImportService {
* Inverse array of SpreadsheetDomain mapping array property. Always use the getter function, which will process the
* property in case it is not set.
*
* @internal
* @see SpreadsheetImportService::getInverseMappingProperties()
* @var array
*/
private $inverseSpreadsheetImportMapping;
private $inverseMappingProperties;
/**
* Identifier properties of SpreadsheetDomain mapping array. Always use the getter function, which will process the
* property in case it is not set.
*
* @internal
* @see SpreadsheetImportService::getMappingIdentifierProperties()
* @var array
*/
private $mappingIdentifierProperties;
/**
* Identifier properties of SpreadsheetDomain argument array. Always use the getter function, which will process the
* property in case it is not set.
*
* @internal
* @see SpreadsheetImportService::getArgumentIdentifierProperties()
* @var array
*/
private $argumentIdentifierProperties;
/**
* Initialize the service before usage with the object to be worked with
*
* @param \WE\SpreadsheetImport\Domain\Model\SpreadsheetImport $spreadsheetImport
*
* @return $this
*/
public function init(SpreadsheetImport $spreadsheetImport) {
$this->inverseSpreadsheetImportMapping = array();
$this->inverseMappingProperties = array();
$this->mappingIdentifierProperties = array();
$this->argumentIdentifierProperties = array();
$this->spreadsheetImport = $spreadsheetImport;
$this->domain = $this->settings[$spreadsheetImport->getContext()]['domain'];
$context = $spreadsheetImport->getContext();
$this->domain = $this->settings[$context]['domain'];
return $this;
}
/**
* Returns the annotation properties of the related domain model with the property name as key.
*
* Example:
* array(['name'] => :Mapping, ['id'] => :Mapping)
*
* @return array
*/
public function getAnnotationMappingProperties() {
......@@ -111,6 +139,8 @@ class SpreadsheetImportService {
}
/**
* Returns the total records of the spreadsheet excluding the header row
*
* @return array
*/
public function getTotalRecords() {
......@@ -121,6 +151,11 @@ class SpreadsheetImportService {
}
/**
* Returns the spreadsheet column as key value pair together with the heading taken of the first row
*
* Example:
* array(['A'] => 'firstname', ['C'] => 'id')
*
* @return array
*/
public function getSpreadsheetColumns() {
......@@ -141,6 +176,8 @@ class SpreadsheetImportService {
}
/**
* Returns a completely mapped new domain object with values taken of the specified row in the spreadsheet.
*
* @param int $number
*
* @return object
......@@ -156,6 +193,9 @@ class SpreadsheetImportService {
}
/**
* Run the import. It inserts, updates and deletes dependent on the configuration of the initialized
* SpreadsheetImport object.
*
* @return void
*/
public function import() {
......@@ -171,7 +211,7 @@ class SpreadsheetImportService {
/** @var \PHPExcel_Worksheet_Row $row */
foreach ($sheet->getRowIterator(2) as $row) {
$totalCount++;
$object = $this->findObjectByIdentifierPropertiesPerRow($row);
$object = $this->findExistingObjectByRow($row);
if (is_object($object)) {
$processedObjectIds[] = $this->persistenceManager->getIdentifierByObject($object);
if ($this->spreadsheetImport->isUpdating()) {
......@@ -204,7 +244,7 @@ class SpreadsheetImportService {
}
$deleteCount = 0;
if ($this->spreadsheetImport->isDeleting()) {
$notExistingObjects = $this->findObjectsByExcludedIds($processedObjectIds);
$notExistingObjects = $this->findObjectsByArgumentsAndExcludedIds($processedObjectIds);
foreach ($notExistingObjects as $object) {
$objectRepository->remove($object);
if (++$deleteCount % $persistRecordsChunkSize === 0) {
......@@ -220,6 +260,8 @@ class SpreadsheetImportService {
}
/**
* Return the active sheet of a spreadsheet. Other potential sheets are ignored on the import.
*
* @return \PHPExcel_Worksheet
*/
private function getFileActiveSheet() {
......@@ -230,41 +272,17 @@ class SpreadsheetImportService {
}
/**
* @return \TYPO3\Flow\Persistence\RepositoryInterface
*/
private function getDomainRepository() {
$repositoryClassName = preg_replace(array('/\\\Model\\\/', '/$/'), array('\\Repository\\', 'Repository'), $this->domain);
/** @var RepositoryInterface $repository */
$repository = $this->objectManager->get($repositoryClassName);
return $repository;
}
/**
* @return array
*/
private function getMappingIdentifierProperties() {
if (empty($this->mappingIdentifierProperties)) {
foreach ($this->spreadsheetImport->getMapping() as $property => $columnMapping) {
/** @var Mapping $mapping */
$mapping = $columnMapping['mapping'];
if ($mapping->identifier) {
$this->mappingIdentifierProperties[$property] = $columnMapping;
}
}
}
return $this->mappingIdentifierProperties;
}
/**
* Returns an existing object found or null for a specific row.
*
* @param \PHPExcel_Worksheet_Row $row
*
* @return null|object
*/
private function findObjectByIdentifierPropertiesPerRow(\PHPExcel_Worksheet_Row $row) {
private function findExistingObjectByRow(\PHPExcel_Worksheet_Row $row) {
$query = $this->getDomainRepository()->createQuery();
$constraints = array();
$identifierProperties = $this->getMappingIdentifierProperties();
foreach ($identifierProperties as $property => $columnMapping) {
$mappingIdentifierProperties = $this->getMappingIdentifierProperties();
foreach ($mappingIdentifierProperties as $property => $columnMapping) {
$column = $columnMapping['column'];
/** @var Mapping $mapping */
$mapping = $columnMapping['mapping'];
......@@ -274,7 +292,10 @@ class SpreadsheetImportService {
$value = $cellIterator->current()->getValue();
$constraints[] = $query->equals($propertyName, $value);
}
$this->mergeQueryConstraintsWithArgumentIdentifiers($query, $constraints);
$argumentIdentifierProperties = $this->getArgumentIdentifierProperties();
foreach ($argumentIdentifierProperties as $property => $value) {
$constraints[] = $query->equals($property, $value);
}
if (!empty($constraints)) {
return $query->matching($query->logicalAnd($constraints))->execute()->getFirst();
} else {
......@@ -282,54 +303,19 @@ class SpreadsheetImportService {
}
}
/**
* @param \TYPO3\Flow\Persistence\QueryInterface $query
* @param array $constraints
*/
private function mergeQueryConstraintsWithArgumentIdentifiers(QueryInterface $query, &$constraints) {
$contextArguments = $this->settings[$this->spreadsheetImport->getContext()]['arguments'];
if (is_array($contextArguments)) {
foreach ($contextArguments as $contextArgument) {
if (isset($contextArgument['identifier']) && $contextArgument['identifier'] == TRUE) {
$name = $contextArgument['name'];
$arguments = $this->spreadsheetImport->getArguments();
if (array_key_exists($name, $arguments)) {
$value = $arguments[$name];
$constraints[] = $query->equals($name, $value);
}
}
}
}
}
/**
* @param array $identifiers
*
* @return \TYPO3\Flow\Persistence\QueryResultInterface
*/
private function findObjectsByExcludedIds(array $identifiers) {
private function findObjectsByArgumentsAndExcludedIds(array $identifiers) {
$query = $this->getDomainRepository()->createQuery();
$constraints[] = $query->logicalNot($query->in('Persistence_Object_Identifier', $identifiers));
$this->mergeQueryConstraintsWithArguments($query, $constraints);
return $query->matching($query->logicalAnd($constraints))->execute();
}
/**
* @param \TYPO3\Flow\Persistence\QueryInterface $query
* @param array $constraints
*/
private function mergeQueryConstraintsWithArguments(QueryInterface $query, &$constraints) {
$contextArguments = $this->settings[$this->spreadsheetImport->getContext()]['arguments'];
if (is_array($contextArguments)) {
foreach ($contextArguments as $contextArgument) {
$name = $contextArgument['name'];
$arguments = $this->spreadsheetImport->getArguments();
if (array_key_exists($name, $arguments)) {
$value = $arguments[$name];
$constraints[] = $query->equals($name, $value);
}
}
// Include all the arguments into the query to get a subset only
foreach ($this->spreadsheetImport->getArguments() as $name => $value) {
$constraints[] = $query->equals($name, $value);
}
return $query->matching($query->logicalAnd($constraints))->execute();
}
/**
......@@ -340,7 +326,7 @@ class SpreadsheetImportService {
/* Set the argument properties before the mapping properties, as mapping property setters might be dependent on
argument property values */
$this->setObjectArgumentProperties($object);
$this->setObjectSpreadsheetImportMappingProperties($object, $row);
$this->setObjectMappingProperties($object, $row);
}
/**
......@@ -369,13 +355,13 @@ class SpreadsheetImportService {
* @param object $object
* @param \PHPExcel_Worksheet_Row $row
*/
private function setObjectSpreadsheetImportMappingProperties($object, $row) {
$inverseSpreadsheetImportMapping = $this->getInverseSpreadsheetImportMapping();
private function setObjectMappingProperties($object, $row) {
$inversedMappingProperties = $this->getInverseMappingProperties();
/** @var \PHPExcel_Cell $cell */
foreach ($row->getCellIterator() as $cell) {
$column = $cell->getColumn();
if (array_key_exists($column, $inverseSpreadsheetImportMapping)) {
$properties = $inverseSpreadsheetImportMapping[$column];
if (array_key_exists($column, $inversedMappingProperties)) {
$properties = $inversedMappingProperties[$column];
foreach ($properties as $propertyMapping) {
$property = $propertyMapping['property'];
/** @var Mapping $mapping */
......@@ -391,16 +377,82 @@ class SpreadsheetImportService {
* Return an inverse SpreadsheetImport mapping array. It flips the property and column attribute and returns it as a
* 3-dim array instead of a 2-dim array. This is done for the case when the same column is assigned to multiple
* properties.
*
* Example:
* array(
* ['A'] => array(
* [0] => array(['property'] => 'name', ['mapping'] => :Mapping),
* [1] => array(['property'] => 'id', ['mapping'] => :Mapping)));
*
* @return array
*/
private function getInverseSpreadsheetImportMapping() {
if (empty($this->inverseSpreadsheetImportMapping)) {
$this->inverseSpreadsheetImportMapping = array();
private function getInverseMappingProperties() {
if (empty($this->inverseMappingProperties)) {
$this->inverseMappingProperties = array();
foreach ($this->spreadsheetImport->getMapping() as $property => $columnMapping) {
$column = $columnMapping['column'];
$propertyMapping = array('property' => $property, 'mapping' => $columnMapping['mapping']);
$this->inverseSpreadsheetImportMapping[$column][] = $propertyMapping;
$this->inverseMappingProperties[$column][] = $propertyMapping;
}
}
return $this->inverseMappingProperties;
}
/**
* Filters the SpreadsheetImport mappings elements for Mapping identifiers only.
*
* Example:
* array(['id'] => array(['column'] => 'A', ['mapping'] => :Mapping)));
*
* @return array
*/
private function getMappingIdentifierProperties() {
if (empty($this->mappingIdentifierProperties)) {
foreach ($this->spreadsheetImport->getMapping() as $property => $columnMapping) {
/** @var Mapping $mapping */
$mapping = $columnMapping['mapping'];
if ($mapping->identifier) {
$this->mappingIdentifierProperties[$property] = $columnMapping;
}
}
}
return $this->mappingIdentifierProperties;
}
/**
* Filters the SpreadsheetImport argument elements for identifier arguments only.
*
* Example:
* array(['id'] => '8df83181-6fb2-11e4-8b37-00505601050c');
*
* @return array
*/
private function getArgumentIdentifierProperties() {
if (empty($this->argumentIdentifierProperties)) {
$context = $this->spreadsheetImport->getContext();
$contextArguments = $this->settings[$context]['arguments'];
if (is_array($contextArguments)) {
foreach ($contextArguments as $contextArgument) {
if (isset($contextArgument['identifier']) && $contextArgument['identifier'] == TRUE) {
$name = $contextArgument['name'];
$arguments = $this->spreadsheetImport->getArguments();
if (array_key_exists($name, $arguments)) {
$this->argumentIdentifierProperties[$name] = $arguments[$name];
}
}
}
}
}
return $this->inverseSpreadsheetImportMapping;
return $this->argumentIdentifierProperties;
}
/**
* @return \TYPO3\Flow\Persistence\RepositoryInterface
*/
private function getDomainRepository() {
$repositoryClassName = preg_replace(array('/\\\Model\\\/', '/$/'), array('\\Repository\\', 'Repository'), $this->domain);
/** @var RepositoryInterface $repository */
$repository = $this->objectManager->get($repositoryClassName);
return $repository;
}
}
Configuration/Testing/Settings.yaml
View file @ c05951cd
WE:
SpreadsheetImport:
testing:
domain: WE\SpreadsheetImport\Tests\Functional\Fixtures\ImportTarget
arguments: ~
domain: WE\SpreadsheetImport\Tests\Functional\Fixtures\Domain\Model\ImportTarget
arguments:
- { name: category, domain: 'WE\SpreadsheetImport\Tests\Functional\Fixtures\Domain\Model\ImportTargetCategory', identifier: TRUE }
- { name: comment, static: 'Sample import' }
Tests/Functional/Fixtures/Domain/Model/ImportTarget.php 0 → 100644
View file @ c05951cd
<?php
namespace WE\SpreadsheetImport\Tests\Functional\Fixtures\Domain\Model;
/* *
* This script belongs to the Flow package "SpreadsheetImport". *
* *
* It is free software; you can redistribute it and/or modify it under *
* the terms of the GNU Lesser General Public License, either version 3 *
* of the License, or (at your option) any later version. *
* *
* The TYPO3 project - inspiring people to share! *
* */
use TYPO3\Flow\Annotations as Flow;
use WE\SpreadsheetImport\Annotations as SpreadsheetImport;
/**
* @Flow\Entity
*/
class ImportTarget {
/**
* @var string
* @SpreadsheetImport\Mapping(identifier=true, setter="setRawId")
*/
protected $id;
/**
* @var string
* @SpreadsheetImport\Mapping
*/
protected $firstName;
/**
* @var string
* @SpreadsheetImport\Mapping
*/
protected $lastName;
/**
* @var string
* @SpreadsheetImport\Mapping
*/
protected $account;
/**
* @var ImportTargetCategory
*/
protected $category;
/**
* @var string
*/
protected $comment;
/**
* @return string
*/
public function getId() {
return $this->id;
}
/**
* @param string $id
*/
public function setId($id) {
$this->id = $id;
}
/**
* @param string $id
*/
public function setRawId($id) {
$this->id = sprintf('%05d', $id);
}
/**
* @return string
*/
public function getFirstName() {
return $this->firstName;
}
/**
* @param string $firstName
*/
public function setFirstName($firstName) {
$this->firstName = $firstName;
}
/**
* @return string
*/
public function getLastName() {
return $this->lastName;
}
/**
* @param string $lastName
*/
public function setLastName($lastName) {
$this->lastName = $lastName;
}
/**
* @return string
*/
public function getAccount() {
return $this->account;
}
/**
* @param string $account
*/
public function setAccount($account) {
$this->account = $account;
}
/**
* @return \WE\SpreadsheetImport\Tests\Functional\Fixtures\Domain\Model\ImportTargetCategory
*/
public function getCategory() {
return $this->category;
}
/**
* @param \WE\SpreadsheetImport\Tests\Functional\Fixtures\Domain\Model\ImportTargetCategory $category
*/
public function setCategory($category) {
$this->category = $category;
}
/**
* @return string
*/
public function getComment() {
return $this->comment;
}
/**
* @param string $comment
*/
public function setComment($comment) {
$this->comment = $comment;
}
}
Tests/Functional/Fixtures/ImportTarget.php → Tests/Functional/Fixtures/Domain/Model/ImportTargetCategory.php
View file @ c05951cd
<?php
namespace WE\SpreadsheetImport\Tests\Functional\Fixtures;
namespace WE\SpreadsheetImport\Tests\Functional\Fixtures\Domain\Model;
/* *
* This script belongs to the Flow package "SpreadsheetImport". *
......@@ -13,22 +13,19 @@ namespace WE\SpreadsheetImport\Tests\Functional\Fixtures;
use TYPO3\Flow\Annotations as Flow;
use WE\SpreadsheetImport\Annotations as SpreadsheetImport;
use WE\SpreadsheetImport\Domain\Model\SpreadsheetImportTargetInterface;
/**
* @Flow\Entity
*/
class ImportTarget {
class ImportTargetCategory {
/**
* @var string
* @SpreadsheetImport\Mapping(identifier=true, setter="setRawId")
*/
protected $id;
/**
* @var string
* @SpreadsheetImport\Mapping
*/
protected $name;
......@@ -46,13 +43,6 @@ class ImportTarget {
$this->id = $id;
}
/**
* @param string $id
*/
public function setRawId($id) {
$this->id = sprintf('%05d', $id);
}
/**
* @return string
*/
......@@ -66,4 +56,5 @@ class ImportTarget {
public function setName($name) {
$this->name = $name;
}
}
Tests/Functional/Fixtures/Domain/Repository/ImportTargetRepository.php 0 → 100644
View file @ c05951cd
<?php
namespace WE\SpreadsheetImport\Tests\Functional\Fixtures\Domain\Repository;
/* *
* This script belongs to the Flow package "SpreadsheetImport". *
* *
* It is free software; you can redistribute it and/or modify it under *
* the terms of the GNU Lesser General Public License, either version 3 *
* of the License, or (at your option) any later version. *
* *
* The TYPO3 project - inspiring people to share! *
* */
use TYPO3\Flow\Annotations as Flow;
use TYPO3\Flow\Persistence\Repository;
/**
* @Flow\Scope("singleton")
*/
class ImportTargetRepository extends Repository {
}
Tests/Functional/SpreadsheetImportServiceTest.php
View file @ c05951cd
......@@ -17,7 +17,8 @@ use TYPO3\Flow\Tests\FunctionalTestCase;
use WE\SpreadsheetImport\Annotations\Mapping;
use WE\SpreadsheetImport\Domain\Model\SpreadsheetImport;
use WE\SpreadsheetImport\SpreadsheetImportService;
use WE\SpreadsheetImport\Tests\Functional\Fixtures\ImportTarget;
use WE\SpreadsheetImport\Tests\Functional\Fixtures\Domain\Model\ImportTarget;
use WE\SpreadsheetImport\Tests\Functional\Fixtures\Domain\Model\ImportTargetCategory;
class SpreadsheetImportServiceTest extends FunctionalTestCase {
......@@ -41,19 +42,17 @@ class SpreadsheetImportServiceTest extends FunctionalTestCase {
$reflectionService = $this->objectManager->get(ReflectionService::class);
$this->inject($this->spreadsheetImportService, 'reflectionService', $reflectionService);
$this->initializeSpreadsheetMock();
}
/**
* @return void
*/
public function tearDown() {
$persistenceManager = self::$bootstrap->getObjectManager()->get('TYPO3\Flow\Persistence\PersistenceManagerInterface');
$persistenceManager = $this->objectManager->get('TYPO3\Flow\Persistence\PersistenceManagerInterface');
if (is_callable(array($persistenceManager, 'tearDown'))) {
$persistenceManager->tearDown();
}
self::$bootstrap->getObjectManager()->forgetInstance('TYPO3\Flow\Persistence\PersistenceManagerInterface');
$this->objectManager->forgetInstance('TYPO3\Flow\Persistence\PersistenceManagerInterface');
parent::tearDown();
}
......@@ -62,47 +61,75 @@ class SpreadsheetImportServiceTest extends FunctionalTestCase {
$spreadsheetImport->setContext('testing');
$resource = $this->resourceManager->importResource(__DIR__ . '/Fixtures/Resources/sample.xlsx');
$spreadsheetImport->setFile($resource);
$idMapping = array('column' => 'C', 'mapping' => new Mapping());
$nameMapping = array('column' => 'A', 'mapping' => new Mapping());
$spreadsheetImport->setMapping(array('id' => $idMapping, 'name' => $nameMapping));
$this->spreadsheetImportService->init($spreadsheetImport);
return $spreadsheetImport;
}
private function initializeConfiguredSpreadsheetMock() {
$spreadsheetImport = $this->initializeSpreadsheetMock();
$annotationMappings = $this->spreadsheetImportService->getAnnotationMappingProperties();
$spreadsheetImport->setMapping(array(
'id' => array('column' => 'C', 'mapping' => $annotationMappings['id']),
'firstName' => array('column' => 'A', 'mapping' => $annotationMappings['firstName']),
'lastName' => array('column' => 'B', 'mapping' => $annotationMappings['lastName']),
'account' => array('column' => 'C', 'mapping' => $annotationMappings['account'])));
$spreadsheetImport->setArguments(array(
'category' => new ImportTargetCategory(), // Could also simply assign the UUID
'comment' => 'Sample import'
));
}
/**
* @test
*/
public function getMappingPropertiesReturnsPropertiesWithMappingAnnotation() {
public function getAnnotationMappingPropertiesReturnsArrayMappingAnnotation() {
$this->initializeSpreadsheetMock();
$properties = $this->spreadsheetImportService->getAnnotationMappingProperties();
$this->assertArrayHasKey('id', $properties);
$this->assertArrayHasKey('name', $properties);
$this->assertArrayHasKey('firstName', $properties);
/** @var Mapping $id */
$id = $properties['id'];
/** @var Mapping $name */
$name = $properties['name'];
/** @var Mapping $firstName */
$firstName = $properties['firstName'];
$this->assertSame(TRUE, $id->identifier);
$this->assertSame(FALSE, $name->identifier);
$this->assertSame(FALSE, $firstName->identifier);
}
/**
* @test
*/
public function getTotalRecordsRecordsNumberOfObjectsToImport() {
$this->initializeSpreadsheetMock();
$this->assertSame(2, $this->spreadsheetImportService->getTotalRecords());
}
/**
* @test
*/
public function getSpreadsheetColumnsReturnsColumnsWithHeadings() {
$this->initializeSpreadsheetMock();
$columns = $this->spreadsheetImportService->getSpreadsheetColumns();
$this->assertArrayHasKey('A', $columns);
$this->assertArrayHasKey('B', $columns);
$this->assertArrayHasKey('C', $columns);
$this->assertContains('name', $columns);
$this->assertContains('lastname', $columns);
$this->assertContains('id', $columns);
$this->assertSame('firstname', $columns['A']);
$this->assertSame('name', $columns['B']);
$this->assertSame('id', $columns['C']);
}
/**
* @test
*/
public function getObjectByRowOneReturnsImportTargetWithSetProperties() {
$this->initializeConfiguredSpreadsheetMock();
/** @var ImportTarget $object */
$object = $this->spreadsheetImportService->getObjectByRow(1);
$this->assertEquals('00001', $object->getId());
$this->assertEquals('Hans', $object->getName());
$this->assertSame('00001', $object->getId());
$this->assertSame('Hans', $object->getFirstName());
$this->assertSame('Muster', $object->getLastName());
$this->assertSame('001', $object->getAccount());
$this->assertInstanceOf(ImportTargetCategory::class, $object->getCategory());
$this->assertSame('Sample import', $object->getComment());
}
}
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment