Github user kamrik commented on the pull request:
https://github.com/apache/cordova-lib/pull/235#issuecomment-110155367
I don't have strong opinion here, but a bit more inclined towards
dependency injection. For some modules like ConfigParser having different
versions imported
Github user kamrik commented on a diff in the pull request:
https://github.com/apache/cordova-lib/pull/235#discussion_r31840378
--- Diff: cordova-lib/src/platforms/PlatformApi.js ---
@@ -0,0 +1,365 @@
+/**
+Licensed to the Apache Software Foundation (ASF) under one
Github user kamrik commented on the pull request:
https://github.com/apache/cordova-lib/pull/235#issuecomment-109401910
Looks great! The need to duplicate common code like ConfgiParser in
cordova-android is a bit annoying and is bound to generate some versioning
confusion
Github user kamrik commented on the pull request:
https://github.com/apache/cordova-windows/pull/84#issuecomment-106447071
Cool, looks much simpler now on the platform side and can let us eventually
get rid of the handler when all platform move to use PlatformApi.
---
If your
Github user kamrik commented on a diff in the pull request:
https://github.com/apache/cordova-windows/pull/84#discussion_r31144880
--- Diff: template/cordova/Api.js ---
@@ -0,0 +1,54 @@
+
+/*jshint node: true*/
+
+var path = require('path');
+
+var buildImpl
Github user kamrik commented on the pull request:
https://github.com/apache/cordova-lib/pull/226#issuecomment-105573860
Looks great!
Looks like figuring out the tests will be pretty difficult. But it might be
worth it just throwing away some of the older tests that rely on a lot
Github user kamrik commented on a diff in the pull request:
https://github.com/apache/cordova-windows/pull/84#discussion_r31052499
--- Diff: template/cordova/Api.js ---
@@ -0,0 +1,54 @@
+
+/*jshint node: true*/
+
+var path = require('path');
+
+var buildImpl
Github user kamrik commented on a diff in the pull request:
https://github.com/apache/cordova-windows/pull/84#discussion_r31053467
--- Diff: template/cordova/Api.js ---
@@ -0,0 +1,54 @@
+
+/*jshint node: true*/
+
+var path = require('path');
+
+var buildImpl
Github user kamrik commented on a diff in the pull request:
https://github.com/apache/cordova-lib/pull/226#discussion_r31048534
--- Diff: cordova-lib/src/cordova/metadata/PlatformHandler.js ---
@@ -0,0 +1,100 @@
+/**
+Licensed to the Apache Software Foundation (ASF
Github user kamrik commented on a diff in the pull request:
https://github.com/apache/cordova-lib/pull/226#discussion_r31049589
--- Diff: cordova-lib/src/cordova/metadata/PlatformHandler.js ---
@@ -0,0 +1,100 @@
+/**
+Licensed to the Apache Software Foundation (ASF
Github user kamrik commented on the pull request:
https://github.com/apache/cordova-lib/pull/210#issuecomment-96304157
Thanks for the pull request.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does
Github user kamrik commented on the pull request:
https://github.com/apache/cordova-lib/pull/205#issuecomment-91273803
LGTM. The scenarios seem convincing. Merging.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your
Github user kamrik commented on the pull request:
https://github.com/apache/cordova-lib/pull/194#issuecomment-86971194
Thanks, merging.
Tests pass ok. Appveyor or GitHub were flaky for the last couple of days.
---
If your project is set up for it, you can reply to this email
Github user kamrik commented on the pull request:
https://github.com/apache/cordova-lib/pull/197#issuecomment-86978847
LGTM. Merging.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have
Github user kamrik commented on the pull request:
https://github.com/apache/cordova-lib/pull/198#issuecomment-86982386
LGTM. Merging.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have
Github user kamrik commented on the pull request:
https://github.com/apache/cordova-lib/pull/177#issuecomment-86993019
Was this all taken care of with #191 ?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your
Github user kamrik commented on the pull request:
https://github.com/apache/cordova-lib/pull/191#issuecomment-86991348
LGTM. Merging.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have
Github user kamrik commented on a diff in the pull request:
https://github.com/apache/cordova-lib/pull/190#discussion_r27117493
--- Diff: cordova-lib/src/platforms/platforms.js ---
@@ -102,5 +102,10 @@ function getPlatformProject(platform, platformRootDir
Github user kamrik commented on a diff in the pull request:
https://github.com/apache/cordova-lib/pull/190#discussion_r27125556
--- Diff: cordova-lib/src/platforms/platforms.js ---
@@ -102,5 +102,10 @@ function getPlatformProject(platform, platformRootDir
Github user kamrik commented on the pull request:
https://github.com/apache/cordova-lib/pull/190#issuecomment-86014944
Will take a look at this today.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does
Github user kamrik commented on the pull request:
https://github.com/apache/cordova-lib/pull/190#issuecomment-86068631
Apparently I had a typo in the GitHub close command in the commit message :)
@TimBarham , could you please close the PR, thanks.
---
If your project is set up
Github user kamrik commented on a diff in the pull request:
https://github.com/apache/cordova-lib/pull/190#discussion_r27128059
--- Diff: cordova-lib/src/platforms/platforms.js ---
@@ -102,5 +102,10 @@ function getPlatformProject(platform, platformRootDir
Github user kamrik commented on the pull request:
https://github.com/apache/cordova-lib/pull/186#issuecomment-81965293
Looks good, merging.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have
Github user kamrik commented on the pull request:
https://github.com/apache/cordova-lib/pull/183#issuecomment-78635997
Addressed the comments and merged as a single squashed commit.
Thanks for reviewing.
---
If your project is set up for it, you can reply to this email and have
Github user kamrik commented on a diff in the pull request:
https://github.com/apache/cordova-lib/pull/183#discussion_r26218885
--- Diff: cordova-lib/src/platforms/platforms.js ---
@@ -0,0 +1,102 @@
+/**
+Licensed to the Apache Software Foundation (ASF) under one
Github user kamrik commented on a diff in the pull request:
https://github.com/apache/cordova-lib/pull/183#discussion_r26218471
--- Diff: cordova-lib/src/plugman/uninstall.js ---
@@ -293,58 +293,25 @@ function runUninstallPlatform(actions, platform,
project_dir, plugin_dir, plugin
Github user kamrik commented on a diff in the pull request:
https://github.com/apache/cordova-lib/pull/183#discussion_r26215310
--- Diff: cordova-lib/src/PluginInfo.js ---
@@ -288,6 +293,19 @@ function PluginInfo(dirname) {
return ret
Github user kamrik commented on a diff in the pull request:
https://github.com/apache/cordova-lib/pull/183#discussion_r26215186
--- Diff: cordova-lib/src/platforms/platforms.js ---
@@ -0,0 +1,102 @@
+/**
+Licensed to the Apache Software Foundation (ASF) under one
Github user kamrik commented on a diff in the pull request:
https://github.com/apache/cordova-lib/pull/183#discussion_r26215211
--- Diff: cordova-lib/src/platforms/platforms.js ---
@@ -0,0 +1,102 @@
+/**
+Licensed to the Apache Software Foundation (ASF) under one
GitHub user kamrik opened a pull request:
https://github.com/apache/cordova-lib/pull/183
CB-8595 Merge platforms.js from cordova plugman CB-8595
Till now we had two separate places for platform specific code
cordova/metadata exposed via cordova/platforms.js
plugman
Github user kamrik commented on the pull request:
https://github.com/apache/cordova-lib/pull/154#issuecomment-71506502
Looks good.
There is a conflict now with the last commit in
spec-cordova/platform.spec.js
Could you please update the PR, thanks.
---
If your project is set
Github user kamrik commented on the pull request:
https://github.com/apache/cordova-lib/pull/51#issuecomment-69623816
At the time I thought the problem of interfering tests would be biting us
much more often but it didn't manifest itself in a while. So I don't think the
extra code
Github user kamrik commented on the pull request:
https://github.com/apache/cordova-lib/pull/131#issuecomment-67004121
This seems to break the tests on Windows. Not sure why. But the ios parser
spec fails on a bunch of mismatches between back and forward slashes in file
paths
Github user kamrik commented on the pull request:
https://github.com/apache/cordova-lib/pull/130#issuecomment-66945316
lgtm, merging
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have
Github user kamrik commented on the pull request:
https://github.com/apache/cordova-plugin-dialogs/pull/46#issuecomment-66841786
Cool, I see you on the list now
http://people.apache.org/committer-index.html
I'll get back to this over the weekend or early next week.
---
If your
Github user kamrik commented on a diff in the pull request:
https://github.com/apache/cordova-plugin-dialogs/pull/46#discussion_r21702976
--- Diff: www/browser/notification.js ---
@@ -0,0 +1,94 @@
+// Platform: browser
+window.navigator.notification
Github user kamrik commented on the pull request:
https://github.com/apache/cordova-plugin-dialogs/pull/46#issuecomment-66686534
James, did you sign the Apache CLA? I don't see you on the list yet. Apache
requires this.
http://people.apache.org/committer-index.html
http
Github user kamrik commented on the pull request:
https://github.com/apache/cordova-lib/pull/123#issuecomment-64693426
A comment inside valid-identifier.js about what this file is for would be
helpful, maybe also about why reserved words are forbidden in IDs. Otherwise
looks good
Github user kamrik commented on the pull request:
https://github.com/apache/cordova-lib/pull/115#issuecomment-64065820
Cool, thanks. Merged.
For the linters, looks like the authors of JSCS and jshint work together
and actually recommend using them both in combination.
https
Github user kamrik commented on the pull request:
https://github.com/apache/cordova-lib/pull/120#issuecomment-63834772
Merging this. Thanks.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have
Github user kamrik commented on the pull request:
https://github.com/apache/cordova-lib/pull/120#issuecomment-63839937
Yes, merged both, it takes some time for them to propagate from Apache git
servers to GitHub.
---
If your project is set up for it, you can reply to this email
Github user kamrik commented on the pull request:
https://github.com/apache/cordova-lib/pull/115#issuecomment-63848626
Looks good overall. 2 style notes, both are optional, tell me if you prefer
not to change it:
1) Would be great if you could run jscs with the default config
Github user kamrik commented on the pull request:
https://github.com/apache/cordova-lib/pull/118#issuecomment-62464427
Looks good. Pushing to master.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does
Github user kamrik commented on the pull request:
https://github.com/apache/cordova-lib/pull/75#issuecomment-52794389
A possible very quick fix: publish a cordova-blackberry10 package to npm
with bundled deps. Solves both problems at once and you won't need neither the
keys nor
Github user kamrik commented on the pull request:
https://github.com/apache/cordova-lib/pull/72#issuecomment-52500410
Thanks for the pull request. Can we make the clone depth even smaller?
Anything prevents us from using depth of 1?
---
If your project is set up for it, you can
Github user kamrik commented on the pull request:
https://github.com/apache/cordova-lib/pull/55#issuecomment-51783588
Problems with opts objects arise all over the place in cordova-lib, I got
bitten by similar problems several times. My proposal would be to create a
CordovaProject
Github user kamrik commented on the pull request:
https://github.com/apache/cordova-lib/pull/55#issuecomment-51841422
I think we should leave this decision to Sergey and the Microsoft guys.
While it would be nice to have CordovaProject class right away, it might take
quite some time
Github user kamrik closed the pull request at:
https://github.com/apache/cordova-lib/pull/69
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature
Github user kamrik commented on the pull request:
https://github.com/apache/cordova-lib/pull/69#issuecomment-51367031
Added the file, some related conversation is here
http://markmail.org/thread/rzzvn2ax3tqzabfw
---
If your project is set up for it, you can reply to this email
Github user kamrik commented on a diff in the pull request:
https://github.com/apache/cordova-lib/pull/55#discussion_r15655705
--- Diff: cordova-lib/src/hooks/scriptsFinder.js ---
@@ -0,0 +1,164 @@
+/**
+ Licensed to the Apache Software Foundation (ASF) under one
GitHub user kamrik opened a pull request:
https://github.com/apache/cordova-lib/pull/69
Add JSCS config file
Don't merge yet - feedback wanted.
JSHint people want to focus on syntax linting and are dropping style
oriented
options. They recommend using JSCS (in addition
Github user kamrik commented on the pull request:
https://github.com/apache/cordova-cli/pull/187#issuecomment-50691123
Looks good.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have
Github user kamrik commented on the pull request:
https://github.com/apache/cordova-cli/pull/186#issuecomment-50524957
Looks good.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have
Github user kamrik commented on the pull request:
https://github.com/apache/cordova-lib/pull/59#issuecomment-49307412
Looks good. What's needed in terms of setting up the hooks on GitHub side?
Do we need in INFRA ticket for this?
---
If your project is set up for it, you can reply
Github user kamrik commented on the pull request:
https://github.com/apache/cordova-lib/pull/61#issuecomment-49363885
Looks good. Merging.
You don't yet have commit rights, right?
---
If your project is set up for it, you can reply to this email and have your
reply appear
Github user kamrik commented on the pull request:
https://github.com/apache/cordova-lib/pull/61#issuecomment-49366276
Yep, I can see you on the ICLA list here
http://people.apache.org/committer-index.html
Merging.
---
If your project is set up for it, you can reply
Github user kamrik commented on a diff in the pull request:
https://github.com/apache/cordova-lib/pull/58#discussion_r14914123
--- Diff: cordova-lib/src/cordova/metadata/android_parser.js ---
@@ -42,6 +42,12 @@ module.exports = function android_parser(project
Github user kamrik commented on a diff in the pull request:
https://github.com/apache/cordova-lib/pull/55#discussion_r14820487
--- Diff: cordova-lib/src/hooks/ScriptsFinder.js ---
@@ -0,0 +1,158 @@
+/**
+ Licensed to the Apache Software Foundation (ASF) under one
Github user kamrik commented on a diff in the pull request:
https://github.com/apache/cordova-lib/pull/55#discussion_r14821829
--- Diff: cordova-lib/src/hooks/ScriptsFinder.js ---
@@ -0,0 +1,158 @@
+/**
+ Licensed to the Apache Software Foundation (ASF) under one
Github user kamrik commented on the pull request:
https://github.com/apache/cordova-lib/pull/51#issuecomment-47931920
I really like the results it produces!
But I'm somewhat surprised by the amount of code. The diff with jasmine
cli.js looks pretty big, which probably means
Github user kamrik commented on the pull request:
https://github.com/apache/cordova-lib/pull/48#issuecomment-47789192
This breaks the specs. @jsoref could you please take a look at this.
The errors are different depending on whether you run npm test or
jasmine-node spec-cordova
Github user kamrik commented on the pull request:
https://github.com/apache/cordova-lib/pull/42#issuecomment-47793655
Looks good, ship it.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have
Github user kamrik commented on the pull request:
https://github.com/apache/cordova-lib/pull/9#issuecomment-47798569
This seems to deal quite a lot with check_requirements(). Considering the
discussion last Thursday, should we first stop using check_requirements() and
then add
Github user kamrik closed the pull request at:
https://github.com/apache/cordova-coho/pull/31
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature
Github user kamrik commented on the pull request:
https://github.com/apache/cordova-coho/pull/31#issuecomment-47352977
Merged.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
GitHub user kamrik opened a pull request:
https://github.com/apache/cordova-coho/pull/31
Add tweaks for releasing cordova-lib
- Don't checkout master in the repos before npm pack-ing them.
- Make coho npm pack cordova-lib like cli and plugman.
- Update instructions
Github user kamrik commented on the pull request:
https://github.com/apache/cordova-lib/pull/30#issuecomment-46688510
The Merge remote... commits are not that horrible, we have plenty of
those in our repos and they have some benefits. But if you want to avoid them,
try some
Github user kamrik commented on the pull request:
https://github.com/apache/cordova-lib/pull/30#issuecomment-46689209
For this particular PR. Thanks for fixing the indentation.
With JSHint added yesterday, npm test now runs JSHint on all the files.
Please run it and fix the JSHint
Github user kamrik commented on the pull request:
https://github.com/apache/cordova-lib/pull/38#issuecomment-46700279
Merged it. But it would be awesome if at this opportunity you could also
replace the direct XML parsing of the plugin.xml with PluginInfo object
Github user kamrik commented on a diff in the pull request:
https://github.com/apache/cordova-cli/pull/181#discussion_r14029357
--- Diff: src/cli.js ---
@@ -165,6 +165,20 @@ function cli(inputArgs) {
throw new CordovaError(msg
Github user kamrik commented on a diff in the pull request:
https://github.com/apache/cordova-cli/pull/181#discussion_r14029489
--- Diff: src/cli.js ---
@@ -165,6 +165,20 @@ function cli(inputArgs) {
throw new CordovaError(msg
Github user kamrik commented on a diff in the pull request:
https://github.com/apache/cordova-cli/pull/181#discussion_r14029634
--- Diff: src/cli.js ---
@@ -165,6 +165,20 @@ function cli(inputArgs) {
throw new CordovaError(msg
Github user kamrik commented on a diff in the pull request:
https://github.com/apache/cordova-lib/pull/30#discussion_r13974255
--- Diff: cordova-lib/src/cordova/metadata/android_parser.js ---
@@ -88,13 +88,54 @@ module.exports.prototype = {
fs.writeFileSync
Github user kamrik commented on the pull request:
https://github.com/apache/cordova-lib/pull/32#issuecomment-46496437
Thanks! This looks much cleaner.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does
Github user kamrik commented on the pull request:
https://github.com/apache/cordova-cli/pull/178#issuecomment-46499213
Hi @gorkem, looks like this functionality was already merged.
Is this correct? If yes, could you please close the pull request.
Same question for https
Github user kamrik commented on the pull request:
https://github.com/apache/cordova-cli/pull/174#issuecomment-46500400
@jsoref, is this still relevant? If yes, could you please rebase it onto
cordova-lib.
Thanks.
---
If your project is set up for it, you can reply to this email
Github user kamrik commented on the pull request:
https://github.com/apache/cordova-cli/pull/174#issuecomment-46502024
Thanks!
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
Github user kamrik commented on the pull request:
https://github.com/apache/cordova-cli/pull/174#issuecomment-46514286
Ok, all merged :)
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have
Github user kamrik commented on the pull request:
https://github.com/apache/cordova-lib/pull/24#issuecomment-45342017
Thanks. Merged that pull request.
Just re-running the tests didn't catch it, even our BuildBot didn't catch
it. I guess this is because npm install didn't
Github user kamrik commented on the pull request:
https://github.com/apache/cordova-lib/pull/24#issuecomment-45280522
@mbektchiev , android parser tests are failing now because they expect
newlines at the ned of each line in the props files, but there are no newlines
after
Github user kamrik commented on a diff in the pull request:
https://github.com/apache/cordova-lib/pull/7#discussion_r13336691
--- Diff: cordova-lib/src/cordova/info.js ---
@@ -31,23 +31,50 @@ var cordova_util = require('./util'),
// Execute using a child_process exec
Github user kamrik commented on the pull request:
https://github.com/apache/cordova-lib/pull/21#issuecomment-44973356
Looks good, will merge later today.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project
Github user kamrik commented on the pull request:
https://github.com/apache/cordova-lib/pull/21#issuecomment-45005347
Merged, but to forgot to add the magic words for the apache robots to
auto-close this pull request. @mbektchiev , could you please close it.
Thanks
Github user kamrik commented on the pull request:
https://github.com/apache/cordova-lib/pull/20#issuecomment-44544621
Looks good. One nit: please use 4 space tabs in cordova-lib.js, for
consistency with most of the other js files in the project.
On a general note, this goes
Github user kamrik commented on the pull request:
https://github.com/apache/cordova-lib/pull/20#issuecomment-44569514
I'm definitely not suggesting to do that right now, it would be a pretty
large change. Was just sharing some thoughts because it's related.
LGTM. Merging
Github user kamrik commented on a diff in the pull request:
https://github.com/apache/cordova-lib/pull/15#discussion_r13134502
--- Diff: cordova-lib/src/plugman/util/config-changes.js ---
@@ -510,8 +509,7 @@ function ConfigFile_load() {
self.data
Github user kamrik commented on a diff in the pull request:
https://github.com/apache/cordova-lib/pull/15#discussion_r13135935
--- Diff: cordova-lib/src/plugman/util/config-changes.js ---
@@ -510,8 +509,7 @@ function ConfigFile_load() {
self.data
Github user kamrik commented on a diff in the pull request:
https://github.com/apache/cordova-lib/pull/21#discussion_r13137628
--- Diff: cordova-lib/src/plugman/platforms/android.js ---
@@ -80,10 +84,115 @@ module.exports = {
},
framework
GitHub user kamrik opened a pull request:
https://github.com/apache/cordova-cli/pull/169
CB-5833: Copy/link to custom merges and config.xml when
When using cordova create with --link-to or --copy-from /some/dir, if
/some/dir
cntains merges or config.xml in addition to www
Github user kamrik commented on a diff in the pull request:
https://github.com/apache/cordova-cli/pull/165#discussion_r11755812
--- Diff: src/save.js ---
@@ -0,0 +1,71 @@
+/**
+Licensed to the Apache Software Foundation (ASF) under one
+or more contributor
Github user kamrik commented on the pull request:
https://github.com/apache/cordova-plugman/pull/72#issuecomment-4544
The best thing would be to avoid the double naming altogether when the
munge is generated, so that it's always config.xml or res/xml/config.xml. If
that's
GitHub user kamrik opened a pull request:
https://github.com/apache/cordova-cli/pull/159
Move e2e tests into spec overwriting duplicate tests.
Merging e2e tests into spec/ dir and removing the old duplicate tests as
discussed in
https://www.mail-archive.com/dev
Github user kamrik commented on the pull request:
https://github.com/apache/cordova-plugman/pull/72#issuecomment-39915727
Looks like the root of the problem is in config_keeper. The files there are
identified by the fake path [see note], so when the same file is referenced
93 matches
Mail list logo
