xmlsquad / gsheet-to-xml

Given the url of a Google Sheet, this Symfony Console command fetches the Google Sheet and outputs it in the form of Xml.
Apache License 2.0
3 stars 1 forks source link

Refactor to isolate model from rest of code. #21

Open forikal-uk opened 6 years ago

forikal-uk commented 6 years ago

Goal

[See title]

Why

To make the gsheet-to-xml more reusable to satisfy User Story: https://github.com/xmlsquad/gsheet-to-xml/issues/20

By extracting model-related code into methods or separate classes, it makes it easier to move the infrastructural parts of the code out into a library.

How

Go through the code and refactor doing "extract method", "Extract superclass", "Form Template Method", etc. with the aim of:

forikal-uk commented 6 years ago

First, I am gonna start at the top and work down (bootstrap -> command -> library)

forikal-uk commented 6 years ago

I have added phpunit to the parent (xml-authoring-project) repo so I can test as I go along.

phpunit should be invoked from the root directory context and, then, explicitly point to the tests folder in gsheet-to-xml package like so:

$ pwd
/Users/x/Documents/Projects/XmlAuthoringSuite/xml-authoring-project
$ ./vendor/phpunit/phpunit/phpunit vendor/xmlsquad/gsheet-to-xml/tests/
PHPUnit 7.2.6 by Sebastian Bergmann and contributors.

.....                                                               5 / 5 (100%)

Time: 47 ms, Memory: 4.00MB

OK (5 tests, 23 assertions)
forikal-uk commented 6 years ago

I looked at the bootstrap.

I refactored a bit. But, there is not much I can do to split the model.

I am wondering if the model should be specified in some kind of config or convention. Anyway, I will think about that as I work.

forikal-uk commented 6 years ago

Refactor the Command script.

forikal-uk commented 6 years ago

Actually, I really ought to be developing with error_reporting set on my php ini.

$ php --ini
Configuration File (php.ini) Path: /etc
Loaded Configuration File:         /etc/php.ini
Scan for additional .ini files in: (none)
Additional .ini files parsed:      (none)

Added:

error_reporting = E_ALL
display_errors = stderr
forikal-uk commented 6 years ago

I noticed that the tests are referencing the old vendor name.

screen shot 2018-06-26 at 16 36 53

Let's change that. Done: https://github.com/xmlsquad/gsheet-to-xml/commit/9f37311d93fb3675c82fb564aff637d5cfa0d620

forikal-uk commented 6 years ago

Looking at the command name.

class GsheetToXmlCommand extends Command
{
    protected function configure()
    {
        $this
            ->setName('inventory:gsheet-to-xml')
...

I think it is time to create an abstract version of the command so that I can move model code into the child class.

Let's do that in a separate task: https://github.com/xmlsquad/gsheet-to-xml/issues/22

forikal-uk commented 6 years ago

I have created the abstract class and moved much of the logic into there.

See: https://github.com/xmlsquad/gsheet-to-xml/issues/22

I wanted to move the createGoogleDriveProcessService() up into the abstract, but I will have to create Interfaces (or abstract classes) for the classes that method uses, so that I can avoid depending on model-related classes which would throw errors.

forikal-uk commented 6 years ago

Before I leave the command classes, I want to ensure that userland user can pass extendable DataSource options to the "GoogleDriveProcessService". i.e. filters, specifications or lookup tables that will be used to augment the resulting xml.

Actually, as I write that. I realise that those kinds of things should be passed to the GoogleDriveProcessService during construction. (Because they live longer than the url and is recursive options).

Either way, it would be handy to push down the invocation to let the developer have control over how the output is written out.

Let's just use the Template Method refactoring to give extendability.

forikal-uk commented 6 years ago

Next I need to https://github.com/xmlsquad/gsheet-to-xml/issues/23

forikal-uk commented 6 years ago

Done

forikal-uk commented 6 years ago

Since creating the AbstractGSheetToXmlCommand::dtoCreateGoogleDriveProcessService() method, I am forced to abstract the XmlSerializer to avoid having to use that concrete class within the abstract command.

forikal-uk commented 6 years ago

Move on to:https://github.com/xmlsquad/gsheet-to-xml/issues/24

forikal-uk commented 6 years ago

So, XmlSerializer has an interface.

forikal-uk commented 6 years ago

Looking at the AbstractGSheetToXmlCommand::doCreateGoogleDriveProcessService() abstract method. I am wondering if this adds too much complexity.

Let's implement this within the abstract class but just allow the user-land developer override to create this in their own way if they want to.

forikal-uk commented 6 years ago

Let's look at removing model from GoogleSpreadsheetReadService

The isHeadingsRow method contains hardcode headings from the model.

It is a private function. Let's see where we can inject them.

The first public method to indirectly invoke it is getSpreadsheetData.

Where is that invoked? In:

OK. So, let's inject that as an array and we can do the Replace Array with Object refactoring on it.

Let's do that in a new issue. #26

forikal-uk commented 6 years ago

Make $headingValues values a property of InventoryFactory

27 done

Move spreadsheet rules to domain object

See: #29 done


The model code has been extracted from GoogleSpreadsheetReadService

forikal-uk commented 6 years ago

Move the common GoogleAPI Services and DomainGSheetObjectFactoryInterface to library

Done.

forikal-uk commented 6 years ago

Convert AbstractGSheetToXmlCommand into just AbstractGSheetProcessingCommand

See #30

forikal-uk commented 6 years ago

Rename XmlSquad\GsheetXml\Model\Service\XmlSerializer to reflect that it is related to a specific domain model.