Join GitHub today
GitHub is home to over 20 million developers working together to host and review code, manage projects, and build software together.
New plugin: gulp #563
Conversation
earnubs
reviewed
Jun 10, 2016
| + | ||
| + - gulp-tasks: | ||
| + (list) | ||
| + A list of dependencies to fetch using npm. |
earnubs
reviewed
Jun 10, 2016
| + }, | ||
| + 'default': [], | ||
| + } | ||
| + schema['properties']['node-engine'] = node_properties['node-engine'] |
earnubs
Jun 10, 2016
Contributor
I wouldn't expect this to be here, I'd expect it only in the nodejs plugin where the deps are also loaded from package.json, no?
earnubs
reviewed
Jun 10, 2016
| + env['PATH'] = '{}:{}'.format( | ||
| + os.path.join(self._npm_dir, 'bin'), env['PATH']) | ||
| + self.run(['npm', 'install', '-g', 'gulp-cli'], env=env) | ||
| + if os.path.exists(os.path.join(self.builddir, 'package.json')): |
earnubs
Jun 10, 2016
Contributor
Aaah, so this is entirely standalone, and there's no need for the nodejs plugin when you use this one. I thought this would be a super simple gulp only plugin (which admittedly is useless without the nodejs plugin).
|
@earnubs thanks for the review |
elopio
reviewed
Jun 10, 2016
| @@ -0,0 +1,106 @@ | ||
| +# -*- Mode:Python; indent-tabs-mode:nil; tab-width:4 -*- | ||
| +# | ||
| +# Copyright (C) 2015 Canonical Ltd |
elopio
reviewed
Jun 10, 2016
| +# You should have received a copy of the GNU General Public License | ||
| +# along with this program. If not, see <http://www.gnu.org/licenses/>. | ||
| + | ||
| +"""The gulp plugin is useful for parts that use gulp tasks. |
elopio
Jun 10, 2016
Member
I had no idea what was gulp a week ago. Is there room here for a short short summary, like:
gulp, the build system that uses node.js streams
elopio
reviewed
Jun 10, 2016
| @@ -0,0 +1,147 @@ | ||
| +# -*- Mode:Python; indent-tabs-mode:nil; tab-width:4 -*- | ||
| +# | ||
| +# Copyright (C) 2015 Canonical Ltd |
|
This is missing one integration test with a gulp simple project. |
elopio
reviewed
Jun 10, 2016
| + | ||
| + patcher = mock.patch('snapcraft.sources.Tar') | ||
| + self.tar_mock = patcher.start() | ||
| + self.addCleanup(patcher.stop) |
elopio
Jun 10, 2016
Member
I'm not sure what tar has to do with this. That means that either it's unnecessary, or missing a comment.
elopio
reviewed
Jun 10, 2016
| + plugin.pull() | ||
| + | ||
| + self.assertFalse(self.run_mock.called, 'run() was called') | ||
| + self.tar_mock.assert_has_calls([ |
elopio
reviewed
Jun 10, 2016
| + os.makedirs(plugin.sourcedir) | ||
| + open(os.path.join(plugin.sourcedir, 'package.json'), 'w').close() | ||
| + | ||
| + with mock.patch.dict(os.environ, {'PATH': 'bin'}): |
elopio
Jun 10, 2016
Member
what if you use the env var fixture, to set PATH for the duration of the test?
elopio
reviewed
Jun 10, 2016
| + 'source-tag', 'source-subdir', 'node-engine'], | ||
| + 'build-properties': ['gulp-tasks'], | ||
| + 'required': ['gulp-tasks'], | ||
| + 'type': 'object'} |
elopio
Jun 10, 2016
Member
@kyrofa tests this with tons of assertions. I have no preference, but I would like the code to be consistent.
kyrofa
Jun 14, 2016
Member
A dump like this makes the tests easier to write, but harder to figure out when the test fails, so I tend to like helpful assertions.
|
FAIL: test_main (test_main.MainTestCase) test_main.MainTestCase.test_maintesttools.testresult.real._StringException: Traceback (most recent call last): |
elopio
reviewed
Jun 14, 2016
| - self.assertEqual(expected, output) | ||
| + binary_output = self.get_output_ignoring_non_zero_exit( |
|
lgtm. I has a missing copyright year update. And there's the nit that 2015-2016 is not valid. It should be 2015, 2016. |
sergiusens
added some commits
Jun 9, 2016
kyrofa
reviewed
Jun 14, 2016
| + | ||
| +"""The gulp plugin drives gulp.js, the streaming build system. | ||
| + | ||
| +The plugin uses gulp to drive the build. It requires a gulpfile.js in |
kyrofa
Jun 14, 2016
Member
"The gulp plugin drives gulp[...]" and "The plugin uses gulp to drive[...]" seem contradictory.
kyrofa
reviewed
Jun 14, 2016
| + A list of gulp tasks to run. | ||
| + - node-engine: | ||
| + (string) | ||
| + The version of nodejs you want the snap to run on. |
kyrofa
Jun 14, 2016
Member
Does "The version of nodejs to be used by the snap" read a little better?
kyrofa
reviewed
Jun 14, 2016
| + 'default': [], | ||
| + } | ||
| + schema['properties']['node-engine'] = node_properties['node-engine'] | ||
| + schema['required'] = ['gulp-tasks'] |
kyrofa
Jun 14, 2016
Member
This removes source from the requirements. Should you append instead, or is that what you intended?
kyrofa
reviewed
Jun 14, 2016
| + | ||
| + # Inform Snapcraft of the properties associated with building. If these | ||
| + # change in the YAML Snapcraft will consider the build step dirty. | ||
| + schema['build-properties'] = ['gulp-tasks'] |
sergiusens
Jun 14, 2016
Collaborator
thanks this is leftover from when I tried to inherit from the nodejs plugin
sergiusens
added some commits
Jun 14, 2016
|
The autopkgtests failed cleaning the test bed at the end, but all the tests passed. So this is good to go, except that it needs to be merged with master. |
|
The autopkgtest failed with "Cannot connect to proxy." in one example test. Not related to this change, so I repeat, good to go. I'm not sure if Kyle has given the other +1, so I'll leave this to Sergio. |
sergiusens
merged commit 00079ee
into
snapcore:master
Jun 15, 2016
sergiusens
deleted the
sergiusens:bugfix/1575880/gulp-went-the
branch
Jun 15, 2016
pushed a commit
to kalikiana/snapcraft
that referenced
this pull request
Apr 6, 2017
sergiusens commentedJun 10, 2016
LP: #1575880
Signed-off-by: Sergio Schvezov sergio.schvezov@ubuntu.com