[GitHub] cordova-lib issue #600: CB-13478 : Fix CRLF line endings and lint integratio...
Github user akdor1154 commented on the issue: https://github.com/apache/cordova-lib/pull/600 is that sufficient? wanted to keep stylistic and behavioural changes in separate commits. --- - To unsubscribe, e-mail: dev-unsubscr...@cordova.apache.org For additional commands, e-mail: dev-h...@cordova.apache.org
[GitHub] cordova-lib pull request #602: CB-12774 : Don't munge scoped plugin IDs anym...
Github user akdor1154 commented on a diff in the pull request: https://github.com/apache/cordova-lib/pull/602#discussion_r147012928 --- Diff: src/cordova/util.js --- @@ -255,11 +255,20 @@ function findPlugins (pluginDir) { var plugins = []; if (fs.existsSync(pluginDir)) { -plugins = fs.readdirSync(pluginDir).filter(function (fileName) { -var pluginPath = path.join(pluginDir, fileName); -var isPlugin = isDirectory(pluginPath) || isSymbolicLink(pluginPath); -return fileName !== '.svn' && fileName !== 'CVS' && isPlugin; -}); +plugins = fs.readdirSync(pluginDir) +.reduce(function (plugins, pluginOrScope) { +if (pluginOrScope[0] === '@') { +plugins.push(...fs.readdirSync(path.join(pluginDir, pluginOrScope)).map(s => path.join(pluginOrScope, s))); --- End diff -- yup just started looking at the failed test :) While you're here and kind of related, I noticed most of the Cordova codebase avoids `() => arrow functions`. Is this just historical? I have used full `function () { }` when I've paid attention just to match code style, but is this necessary? --- - To unsubscribe, e-mail: dev-unsubscr...@cordova.apache.org For additional commands, e-mail: dev-h...@cordova.apache.org
[GitHub] cordova-lib pull request #602: CB-12774 : Don't munge scoped plugin IDs anym...
GitHub user akdor1154 opened a pull request: https://github.com/apache/cordova-lib/pull/602 CB-12774 : Don't munge scoped plugin IDs anymore. Comment requested. In order to get plugins that are under an npm scope (e.g. `@akdor1154/some-plugin`) to install, a previous change from took the approach to consider such a plugin to be called (have the id) `some-plugin` instead of `@akdor1154/some-plugin`. This allowed scoped plugins to install and preserved the assumption that plugins will always be installed in `plugins_dir/[plugin_id]`. However, it required special parsing logic around `npm` package IDs, and it broke the assumption that the `name` in an npm plugin's `package.json' would correspond to Cordova's idea of a plugin ID. IMO this approach is the source of further complexity which is not required, and is leading to weird bugs and special cases with scoped plugins. (see the linked issue, but there is stuff as basic as "`npm install` no longer works after installing a scoped plugin") This PR changes approach - such plugins are now considered to have the id `@akdor1154/some-plugin`, in agreement with how npm treats such packages. This allows almost all special cases for scoped packages to be removed (yay). The key difference in behaviour as a result of this, though, is that while plugins are still installed in `plugins_dir/[plugin_id]`, `[plugin_id]` may no longer be a single directory, leading to plugin directories that look like ``` plugins |- @akdor1154 | |- some-plugin |- cordova-some-other-plugin ``` Most of the logic changes in this PR are based around making this change work. It's largely done how I want it, but I guess maintainers probably have strong opinions over whether this is the right way to go or not. Because of this I've left some commits in marked as TEMP that are for my own workflow. Please keep in mind I'll remove these. The only one you should be mindful of is the monkey patch to `cordova-common`; this would need to be raised in a separate PR to cordova-common if this change was approved in principle. ### Platforms affected All ### What testing has been done on this change? New end-to-end integration tests including a proper scoped plugin fixture Unit tests on scoped plugins Unit tests for plugman metadata ### Checklist - [x] [Reported an issue](http://cordova.apache.org/contribute/issues.html) in the JIRA database - [x] Commit message follows the format: "CB-3232: (android) Fix bug with resolving file paths", where CB- is the JIRA ID & "android" is the platform affected. - [x] Added automated test coverage as appropriate for this change. You can merge this pull request into a Git repository by running: $ git pull https://github.com/akdor1154/cordova-lib simple-scopes Alternatively you can review and apply these changes as the patch at: https://github.com/apache/cordova-lib/pull/602.patch To close this pull request, make a commit to your master/trunk branch with (at least) the following in the commit message: This closes #602 commit 9c1c320d72bf2860f95bd9330ffd7718cdabba82 Author: Jarrad Whitaker <jwhita...@officeworks.com.au> Date: 2017-10-20T06:55:44Z change scope behaviour to consider scopes to be part of the package name commit 67dbb75fa96de8486418dd0a0bfbaa22300690d5 Author: Jarrad Whitaker <jwhita...@officeworks.com.au> Date: 2017-10-20T07:07:56Z remove extraneous done callback from scope plugin tests commit c6f681cc4df5fd4a5bd246df00a62753244c Author: Jarrad Whitaker <jwhita...@officeworks.com.au> Date: 2017-10-20T08:40:08Z add scoped plugin testcase commit c2346e062518c639381ac167ba60a169500501eb Author: Jarrad Whitaker <jwhita...@officeworks.com.au> Date: 2017-10-20T08:41:46Z allow scoped plugins to exist in a dir structure reflecting there name, c.f. npm commit 14c3f74a0d9033dfb250bc7b698e18eb226bf544 Author: Jarrad Whitaker <jwhita...@officeworks.com.au> Date: 2017-10-20T08:42:07Z add an integration test to check scoped plugin add+remove commit 0e383f9b419c5aa22bbacb456c0ff38649e1 Author: Jarrad Whitaker <jwhita...@officeworks.com.au> Date: 2017-10-20T08:43:14Z TEMP lint with typescript commit 7018c13a635b46364b34cf4378c78c60a1012f8b Author: Jarrad Whitaker <jwhita...@officeworks.com.au> Date: 2017-10-20T08:42:27Z TEMP override plugin discovery in cordova-common commit 7aad7297f7306b380761e01017c067b31beca4a7 Author: Jarrad Whitaker <jwhita...@officeworks.com.au> Date: 2017-10-23T06:23:14Z fix get_fetch_metadata to not guess plugins_dir anymore commit 17a8cce029ede8060383f041bbc9735f841556bf Author: Jarrad Whitaker <jwhita...@officeworks.com.au> Date: 2017-10-23T06:53:59Z remove top_plugins override commit c0b1f5
[GitHub] cordova-lib issue #601: CB-13485: Test with Node8
Github user akdor1154 commented on the issue: https://github.com/apache/cordova-lib/pull/601 Ah, don't we love npm 5? I didn't expect this to break so early, I'll look tomorrow. --- - To unsubscribe, e-mail: dev-unsubscr...@cordova.apache.org For additional commands, e-mail: dev-h...@cordova.apache.org
[GitHub] cordova-lib pull request #601: CB-13485: Test with Node8
GitHub user akdor1154 opened a pull request: https://github.com/apache/cordova-lib/pull/601 CB-13485: Test with Node8 ### What does this PR do? Enables testing on Node 8 with Travis, and fixes the failing tests (no behaviour changes seem necessary). ### What testing has been done on this change? `npm test` ### Checklist - [x] [Reported an issue](http://cordova.apache.org/contribute/issues.html) in the JIRA database - [x] Commit message follows the format: "CB-3232: (android) Fix bug with resolving file paths", where CB- is the JIRA ID & "android" is the platform affected. - [x] Added automated test coverage as appropriate for this change. You can merge this pull request into a Git repository by running: $ git pull https://github.com/akdor1154/cordova-lib node8 Alternatively you can review and apply these changes as the patch at: https://github.com/apache/cordova-lib/pull/601.patch To close this pull request, make a commit to your master/trunk branch with (at least) the following in the commit message: This closes #601 commit 8e38e274cdd140717766fc43a4b28934f4106e64 Author: Jarrad Whitaker <jwhita...@officeworks.com.au> Date: 2017-10-23T23:38:55Z Test Node 8 with Travis commit 0a704ab17d420273d95c6ef3cc7b7774d6f3 Author: Jarrad Whitaker <jwhita...@officeworks.com.au> Date: 2017-10-23T23:51:26Z fix error test on Node 8 --- - To unsubscribe, e-mail: dev-unsubscr...@cordova.apache.org For additional commands, e-mail: dev-h...@cordova.apache.org
[GitHub] cordova-lib pull request #600: CB-13478 : Fix CRLF line endings and lint int...
GitHub user akdor1154 opened a pull request: https://github.com/apache/cordova-lib/pull/600 CB-13478 : Fix CRLF line endings and lint integration tests. ### Platforms affected N/A ### What does this PR do? - Change CRLFs to LFs - Configures eslint to whinge in future if CRLFs sneak back in - Adds gitattributes to prevent this in future - Adds integration-tests to the eslint script, and fixes lint errors it found in there (mainly line endings). ### What testing has been done on this change? `npm run eslint` ### Checklist - [x] [Reported an issue](http://cordova.apache.org/contribute/issues.html) in the JIRA database - [x] Commit message follows the format: "CB-3232: (android) Fix bug with resolving file paths", where CB- is the JIRA ID & "android" is the platform affected. - [x] Added automated test coverage as appropriate for this change. You can merge this pull request into a Git repository by running: $ git pull https://github.com/akdor1154/cordova-lib lf Alternatively you can review and apply these changes as the patch at: https://github.com/apache/cordova-lib/pull/600.patch To close this pull request, make a commit to your master/trunk branch with (at least) the following in the commit message: This closes #600 commit 87ec94735455976130e2ec0dcaa0d2d72b2995de Author: Jarrad Whitaker <akdor1...@gmail.com> Date: 2017-10-21T00:25:36Z eslint and gitattributes to ensure LF commit 878c5ce45523223f3fe1c1b731367c66f87b7dcb Author: Jarrad Whitaker <akdor1...@gmail.com> Date: 2017-10-21T00:27:01Z crlf -> lf with eslint commit ce8157585db744bce82cd6f1b5224f58aa948f68 Author: Jarrad Whitaker <akdor1...@gmail.com> Date: 2017-10-21T00:29:29Z lint integration tests too commit 4a5ce48dd0c7ded0429e3655c2ec0eb4e58bb2f2 Author: Jarrad Whitaker <akdor1...@gmail.com> Date: 2017-10-21T00:32:26Z crlf -> lf in integration tests commit 4c6ef8a98c9147a992c70ed015fe5eed28be944e Author: Jarrad Whitaker <akdor1...@gmail.com> Date: 2017-10-21T00:33:27Z eslint --fix in integration-tests commit afee24c3e6ffe3c65015abd6180aee464434ce76 Author: Jarrad Whitaker <akdor1...@gmail.com> Date: 2017-10-21T00:34:37Z eqeqeq in HooksRunner --- - To unsubscribe, e-mail: dev-unsubscr...@cordova.apache.org For additional commands, e-mail: dev-h...@cordova.apache.org