Manifest file strategy #15

pjcdawkins · 2015-12-16T18:17:13Z

As described in #13

Minor todos, not sure if you'd want them:

add setter methods to match current strategies a bit more closely
what about all the copyright/authorship stuff in the file docblock

padraic · 2016-01-05T18:57:22Z

@pjcdawkins Thanks for the PR! I'll review over the next week time allowing.

On your two questions:

I'd keep setter names as consistent as possible with existing API (where it makes sense).
Best practice is to include the license block at file head. Also, by all means include your name for author and copyright credit. I do not require copyright assignment, only a compatible license. If the code includes any pre-existing code, do include a line indicating the original source, author and copyright to ensure we abide by the terms of any prevailing license it may fall under, e.g. "Adapted from xxx ". It will also let me double check we're legal ;).

aik099 · 2016-05-31T11:07:17Z

Apparently the review stage took longer than expected.

pjcdawkins · 2016-05-31T11:12:01Z

Ah yes... I also have extended this a bit where I'm using it (in the Platform.sh CLI), to add support for timeouts, for upgrade notes, and for PHP version requirements:
https://github.com/platformsh/platformsh-cli/blob/development/src/SelfUpdate/ManifestStrategy.php

I'll roll that into an update for this PR, if there is still interest.

ArnaudLigny · 2016-06-06T21:33:33Z

I'll roll that into an update for this PR, if there is still interest.

👍

padraic · 2017-04-14T17:59:15Z

Sorry for the delay folks - my review times suck recently 😁 We'll work around to this much faster this time.

theofidry

I'm not very familiar with the codebase yet so the technical review is somewhat lacking, but you'll find some comments still :)

theofidry · 2017-04-14T19:32:07Z

src/Strategy/ManifestStrategy.php

+use Humbug\SelfUpdate\Updater;
+use Humbug\SelfUpdate\VersionParser;
+
+class ManifestStrategy implements StrategyInterface


I would make this class final

theofidry · 2017-04-14T19:33:36Z

src/Strategy/ManifestStrategy.php

+    private $allowUnstable = false;
+
+    /**
+     * ManifestStrategy constructor.


I'm personally not very found of noisy comments (i.e. comment not providing any value), so I would remove that line unless @padraic prefer to keep it for consistency. Although I would argue in favour of changing that practice and fixing that inconsistency later :)

Opinions can vary, but I'm fairly verbose myself in comments. I just find it a PITA, esp. in open source projects, to spend too time familiarising myself with how something works so a bit of hand holding can be welcome.

Also, easy to copy/paste to a README and THEN delete the comments ;).

theofidry · 2017-04-14T19:34:28Z

src/Strategy/ManifestStrategy.php

+    private $localVersion;
+    /** @var bool */
+    private $allowMajor = false;
+    /** @var bool */


let's keep it like:

/** * @var bool */ private $allowUnstable = false;

theofidry · 2017-04-14T19:35:10Z

src/Strategy/ManifestStrategy.php

+        if ($version === false) {
+            throw new \RuntimeException('No remote versions found');
+        }
+        $versionInfo = $this->getAvailableVersions();


missing blank line between L63-L64

theofidry · 2017-04-14T19:36:15Z

src/Strategy/ManifestStrategy.php

+    }
+
+    /**
+     * Get available versions to update to.


theofidry · 2017-04-14T19:43:55Z

tests/Humbug/Test/SelfUpdate/UpdaterManifestStrategyTest.php

+
+class UpdaterManifestStrategyTest extends \PHPUnit_Framework_TestCase
+{
+


unnecessary blank line

theofidry · 2017-04-14T19:51:56Z

src/Strategy/ManifestStrategy.php

+                throw new \RuntimeException(sprintf('Failed to download manifest: %s', $this->manifestUrl));
+            }
+
+            $this->manifest = (array) json_decode($manifestContents, true);


$this->manifest = json_decode($manifestContents, true, 512, JSON_OBJECT_AS_ARRAY);

theofidry · 2017-04-14T19:54:27Z

src/Strategy/ManifestStrategy.php

+            }
+        }
+
+        return $this->manifest;


I would rather return a $manifest property and do $this->manifest = $this->retrieveManifest()

theofidry · 2017-04-14T19:54:46Z

tests/Humbug/Test/SelfUpdate/UpdaterManifestStrategyTest.php

+class UpdaterManifestStrategyTest extends \PHPUnit_Framework_TestCase
+{
+
+    private $files;


missing phpdoc for the props

theofidry · 2017-04-14T19:55:32Z

tests/Humbug/Test/SelfUpdate/UpdaterManifestStrategyTest.php

+        chdir($cwd);
+    }
+
+    public function teardown()


I would move it just under setUp(), they go together :)

and also there's a missing @inheritdoc for both setUp() and tearDown()

padraic · 2017-05-12T23:58:16Z

Note: Will complete a refresh PR over the weekend - we don't have write access back to the originating branch so will be a basic merge, resolve review pts, and check the additional work noted in comments.

padraic · 2017-05-14T17:37:56Z

@pjcdawkins I've updated this PR in #37 and it should be compatible with your final version. The API has been altered to prefer setters, and SHA1 has been moved to an optional downgrade from a SHA-256 default, but otherwise the overall internals are unchanged. If/When #37 is merged, this PR will be closed. Thanks again for opening this PR!

pjcdawkins force-pushed the manifest-strategy branch from ff7c53a to 2ec6c44 Compare December 16, 2015 21:57

pjcdawkins added 2 commits December 16, 2015 22:03

Add ManifestStrategy

a241016

Write ManifestStrategy tests

a35b4c1

pjcdawkins force-pushed the manifest-strategy branch from 2ec6c44 to a35b4c1 Compare December 16, 2015 22:03

theofidry suggested changes Apr 14, 2017

View reviewed changes

padraic mentioned this pull request May 13, 2017

Manifest file strategy (updated) #37

Open

8 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Manifest file strategy #15

Manifest file strategy #15

pjcdawkins commented Dec 16, 2015

padraic commented Jan 5, 2016

aik099 commented May 31, 2016

pjcdawkins commented May 31, 2016 •

edited

Loading

ArnaudLigny commented Jun 6, 2016

padraic commented Apr 14, 2017

theofidry left a comment

theofidry Apr 14, 2017

theofidry Apr 14, 2017

padraic May 12, 2017

theofidry Apr 14, 2017

theofidry Apr 14, 2017

theofidry Apr 14, 2017

theofidry Apr 14, 2017

theofidry Apr 14, 2017

theofidry Apr 14, 2017

theofidry Apr 14, 2017

theofidry Apr 14, 2017

padraic commented May 12, 2017

padraic commented May 14, 2017


		class UpdaterManifestStrategyTest extends \PHPUnit_Framework_TestCase
		{

Manifest file strategy #15

Are you sure you want to change the base?

Manifest file strategy #15

Conversation

pjcdawkins commented Dec 16, 2015

padraic commented Jan 5, 2016

aik099 commented May 31, 2016

pjcdawkins commented May 31, 2016 • edited Loading

ArnaudLigny commented Jun 6, 2016

padraic commented Apr 14, 2017

theofidry left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

padraic commented May 12, 2017

padraic commented May 14, 2017

pjcdawkins commented May 31, 2016 •

edited

Loading