no no! linting is good / I think that should def be considered broken the fast/loose style of 'approve anything that doesn't break the tests' made a lot of sense in our early days. this is a (mostly) mature project now and I think driving to consistency in our code would be a very good thing.
On Mon, Apr 28, 2014 at 11:59 AM, Andrew Grieve <[email protected]>wrote: > I like that style is enforced with a tool, but I wouldn't be bothered if > was removed. > > > On Mon, Apr 28, 2014 at 1:27 PM, Jesse <[email protected]> wrote: > > > The fact that indentation can break the build is troubling to me. Am I > > alone? > > > > @purplecabbage > > risingj.com > > > > > > On Mon, Apr 28, 2014 at 9:36 AM, Andrew Grieve <[email protected]> > > wrote: > > > > > I didn't revert it right away, I sent an email saying that I'd do it in > > the > > > morning if it wasn't fixed, and then did so. It's much worse to have a > > > broken build than it is to just revert. It's *very* easy to revert a > > revert > > > commit, and it takes the pressure off the person who broke it to come > up > > > with a fix quickly. > > > > > > Agree I could have just fixed the indents. I didn't realize that was > the > > > only thing that was wrong with it. > > > > > > > > > > > > > > > On Sat, Apr 26, 2014 at 1:25 AM, Anis KADRI <[email protected]> > > wrote: > > > > > > > On Fri, Apr 25, 2014 at 7:35 PM, Michal Mocny <[email protected]> > > > wrote: > > > > > > > > > On Fri, Apr 25, 2014 at 10:20 PM, purplecabbage < > > > [email protected] > > > > > >wrote: > > > > > > > > > > > I believe the plan was, merge now to save headaches later *but* > > also > > > > hide > > > > > > the browserify functionality behind a flag, so it could co-exist. > > > > > > > > > > > > Sent from my iPhone > > > > > > > > > > > > > On Apr 25, 2014, at 5:40 PM, Anis KADRI <[email protected]> > > > > wrote: > > > > > > > > > > > > > > On Fri, Apr 25, 2014 at 11:05 AM, Andrew Grieve < > > > > [email protected] > > > > > > >wrote: > > > > > > > > > > > > > >> On Fri, Apr 25, 2014 at 1:35 PM, Michal Mocny < > > > [email protected]> > > > > > > wrote: > > > > > > >>>> On Fri, Apr 25, 2014 at 1:27 PM, Anis KADRI < > > > [email protected] > > > > > > > > > > > >>> wrote: > > > > > > >>> > > > > > > >>>> Contacts is known to not work because some modules > > > > > > >>>> require('./ContactError') instead of > > > > > > >>>> ('org.apache.cordova.contacts.ContactError') like most other > > > > modules > > > > > > >> do. I > > > > > > >>>> didn't bother to fix yet it because it was an exception not > a > > > > rule. > > > > > > >>>> I didn't try any of the chrome plugins. They might be doing > > the > > > > same > > > > > > >> sort > > > > > > >>>> of thing. > > > > > > >> > > > > > > >> Just wanted to be clear here that "fix" means allow the > > > "./syntax", > > > > > > >> not convert to abs syntax. File plugin uses this extensively, > > and > > > > > > >> we've supported it for quite a while. > > > > > > > > > > > > > > Thanks for the heads up but require('path') works. The only > > > > difference > > > > > is > > > > > > > that it actually checks if the path exists and if it doesn't > then > > > it > > > > > > fails. > > > > > > > Which makes this file [1] fail because it's requiring a module > > that > > > > is > > > > > > > present in the parent directory (../ContactError) while > > referencing > > > > it > > > > > as > > > > > > > if it was in the same directory (./ContactError). Are chrome > > > plugins > > > > > > doing > > > > > > > a similar thing ? File/FileTransfer are not concerned because > the > > > > paths > > > > > > > they reference do exist. > > > > > > > > > > > > > > > > > > > > >> > > > > > > >> > > > > > > >>>> I didn't noticed any major performance issues when comparing > > old > > > > > > plugman > > > > > > >>>> and new plugman with browserify. I am not using CLI though. > > > > > > >> > > > > > > >> As a litmus test, I'd suggest running the createmobilespec.js > > > script > > > > > > >> within mobile-spec. > > > > > > > > > > > > > > I will try this out. The reason it takes longer is partly > because > > > > it's > > > > > > > generating 2 bundles. I am sure there can be ways to improve > > > > > performance > > > > > > > though. I will keep looking into it. > > > > > > > > > > > > > > > > > > > > >> > > > > > > >>>> Thanks for giving me the time to look into this before > > reverting > > > > my > > > > > > >>>> changes. Much appreciated. Besides reverting cordova-js was > > > > > completely > > > > > > >>>> unnecessary because I only added a task to compile a > > browserify > > > > > bundle > > > > > > >>>> (everything else was not touched). > > > > > > >> > > > > > > >> I think it's important to keep the continuous build green, and > > > > > > >> cordova-js was broken: > > > > > > >> https://issues.apache.org/jira/browse/CB-6515 > > > > > > >> > > > > > > >> To re-do your changes, just git revert the revert commit and > > carry > > > > on. > > > > > > >> It's important that everyone else isn't blocked by master > being > > > > > > >> broken. > > > > > > > > > > > > > > jshint was failling because I used 2 indentations instead of 4. > > > > Indeed, > > > > > > > that is absolutely unacceptable. > > > > > > > > > > > > I suggest that we make indentation our top priority going > forward. > > > > Those > > > > > > > types of bug that users report that make them unable to compile > > > their > > > > > > > projects can just sit in there and wait after all > > > > > > > (CB-6441<https://issues.apache.org/jira/browse/CB-6441>). > > > > > > > Who cares about real users as long as the continuous build > screen > > > is > > > > > > green, > > > > > > > right? > > > > > > > > > > > > > > > > > > > > > Why did you include that fix as part of this unrelated work? If > you > > > > > squashed the browserify work it would have been easier to pick out > > more > > > > > specific reverts. (additionally there is duplication of all the > > > > browserify > > > > > commits if you look at the git log, not sure how that happened, so > > > revert > > > > > of anything but the whole ). > > > > > > > > > > > > Could you please clarify your thoughts? I don't get this part. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > I fixed the indentation which makes jshint happy. I merged the > > > > > browserify > > > > > > > branch back to master (with the critical indentation fix). You > do > > > let > > > > > me > > > > > > > know if your continuous build is bleeding again. > > > > > > > > > > > > > > > > Dude its not "your" CI, its our CI. It catches real issues weekly, > > and > > > > > cannot do that if its red. > > > > > > > > > > > > > Ownership doesn't matter. I never criticized the CI and same as you, > I > > > > think it can be really useful. I just said that what it caught this > > time > > > > was absolute BS and it would have been easier to fix it than to > revert > > > > everything. So the first reflex when it's red should always be > revert ? > > > No > > > > chance/time to fix ? It took me less 20 seconds (ok maybe a minute). > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > Finally, you asked me at the last hangout to merge everything > to > > > > master > > > > > > in > > > > > > > order to survive the breakout of cordova-cli and > cordova-plugman > > > and > > > > > that > > > > > > > is why I took the liberty to do it sooner rather than later. > Also > > > we > > > > > need > > > > > > > to start testing this stuff at some point and it's not like > > nothing > > > > has > > > > > > > ever been broken with the project. > > > > > > > > > > > > > > > > ^ What Jesse said. > > > > > > > > > > > > > What Jesse said is already there for cordova-js (there is a compile > and > > > > compile-browserify task that don't overlap in any way). There is no > > flag > > > > for plugman and perhaps there should be. That is a valid point. So I > > will > > > > make it available behind a flag and we'll go from there. But you see > > > these > > > > are the discussions that I was hoping to trigger when I sent this > > > message. > > > > I was not expecting a: "!@#% it, it's broken, let's revert cuz I > don't > > > want > > > > to see my CI bleed more than 5 minutes". What was the urgency anyway > ? > > > Were > > > > we releasing today? > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > [1] > > > > > > > > > > > > > > > > > > > > > > > > > > > > https://git-wip-us.apache.org/repos/asf?p=cordova-plugin-contacts.git;a=blob;f=www/ios/Contact.js;h=b40c41acc5c78e2233ca434388ab012cd635fba9;hb=HEAD > > > > > > > > > > > > > > > > > > > > >> > > > > > > >>> > > > > > > >>> All the fixes will probably take more than an hour, and > > shouldn't > > > > > block > > > > > > >>> master. Why the rush to merge in? > > > > > > >>> > > > > > > >>> > > > > > > >>>> > > > > > > >>>> > > > > > > >>>> > > > > > > >>>>> On Fri, Apr 25, 2014 at 8:13 AM, Andrew Grieve < > > > > > [email protected] > > > > > > >>>> wrote: > > > > > > >>>> > > > > > > >>>>> reverted the merge commits to plugman & js. > > > > > > >>>>> > > > > > > >>>>> On Fri, Apr 25, 2014 at 9:43 AM, Michal Mocny < > > > > [email protected] > > > > > > > > > > > > >>>>> wrote: > > > > > > >>>>>> Also Anis, would it please be possible to squash most of > > these > > > > > > >> commits > > > > > > >>>>> into > > > > > > >>>>>> a few units of standalone work? Probably best way to do > > that > > > is > > > > > > >> create > > > > > > >>>>> a > > > > > > >>>>>> new local branch from your old branch point, apply the > diffs > > > > from > > > > > > >>>>>> browserify branch, rebase -i them, then push that branch > > (this > > > > way > > > > > > >> you > > > > > > >>>>>> don't need to push with force). > > > > > > >>>>>> > > > > > > >>>>>> -Michal > > > > > > >>>>>> > > > > > > >>>>>> > > > > > > >>>>>>> On Fri, Apr 25, 2014 at 9:30 AM, Michal Mocny < > > > > > [email protected] > > > > > > > > > > > > > >>>>>> wrote: > > > > > > >>>>>> > > > > > > >>>>>>> > > > > > > >>>>>>> > > > > > > >>>>>>> > > > > > > >>>>>>>> On Fri, Apr 25, 2014 at 12:53 AM, Michal Mocny < > > > > > > [email protected] > > > > > > >>>>>>> wrote: > > > > > > >>>>>>> > > > > > > >>>>>>>> TLDR; Ran a bunch of experiments tonight. I think this > is > > > too > > > > > > >> early > > > > > > >>>>> to > > > > > > >>>>>>>> merge into master. We pointed out several issues in > > > previous > > > > > > >> threads > > > > > > >>>>> and > > > > > > >>>>>>>> seems they were ignored. > > > > > > >>>>>>>> > > > > > > >>>>>>>> Few quick comments from trying this: > > > > > > >>>>>>>> - cordova-js is a new dependency of plugman, and needs > to > > be > > > > npm > > > > > > >>>>> linked > > > > > > >>>>>>>> to local dev version > > > > > > >>>>>>>> - Seems we run browserify after each plugin add > (possibly > > > due > > > > to > > > > > > an > > > > > > >>>>>>>> auto-prepare?) so creating projects like mobilespec or > any > > > > > mobile > > > > > > >>>>> chrome > > > > > > >>>>>>>> app is now *much* slower (measured in minutes) > > > > > > >>>>>>>> - Each cordova prepare now takes 6.5s on a very small > > > project > > > > > :'( > > > > > > >>>>>>> (reverted changes locally, old prepare takes <0.5s on > same > > > > > project, > > > > > > >>>>> which > > > > > > >>>>>>> is still slow!) > > > > > > >>>>>>> > > > > > > >>>>>>> > > > > > > >>>>>>>> Things that are currently found broken: > > > > > > >>>>>>>> - prepare step fails for my cordova testing application > > > after > > > > > > >>>>>>>> installing org.apache.cordova.contacts, and > > > > > > >>>>>>>> - prepare step fails for *all* cca apps because of same > > > error > > > > as > > > > > > >>>>> above, > > > > > > >>>>>>>> but for chrome.runtime plugin :( > > > > > > >>>>>>>> - These issues seem due to js-modules not being > > > browserify-ed > > > > > > >>>>> properly. > > > > > > >>>>>>>> It may be that both are bad modules (?), but it used to > > work > > > > > fine! > > > > > > >>>>>>>> > > > > > > >>>>>>>> I did get a few apps running fine, so at least we got > that > > > > going > > > > > > >> for > > > > > > >>>>> us ;) > > > > > > >>>>>>>> > > > > > > >>>>>>>> Still to do: > > > > > > >>>>>>>> - track impact tp startup time > > > > > > >>>>>>>> - see if there aren't any plugins with subtle bugs due > to > > > > > > auto-runs > > > > > > >>>>>>>> behaviour > > > > > > >>>>>>>> > > > > > > >>>>>>>> -Michal > > > > > > >>>>>>>> > > > > > > >>>>>>>> > > > > > > >>>>>>>> On Thu, Apr 24, 2014 at 9:34 PM, Andrew Grieve < > > > > > > >> [email protected] > > > > > > >>>>>> wrote: > > > > > > >>>>>>>> > > > > > > >>>>>>>>> Cool! Does no impact mean that browserify is still not > > used > > > > by > > > > > > >>>>>>>>> default, or does it mean that it's backward compatible? > > > > > > >>>>>>>>> > > > > > > >>>>>>>>> Failing specs sounds like impact... > > > > > > >>>>>>>>> > > > > > > >>>>>>>>> And it does look like medic is failing due to > > > browserify-type > > > > > > >> things: > > > > > > >>>>>>>>> http://108.170.217.131:8010/waterfall > > > > > > >>>>>>>>> > > > > > > >>>>>>>>> Unless you feel like powering through this tonight, > I'll > > > > > probably > > > > > > >>>>>>>>> revert in the morning so that our continuous build can > > stay > > > > > > green. > > > > > > >>>>>>>>> > > > > > > >>>>>>>>>> On Thu, Apr 24, 2014 at 6:06 PM, Brian LeRoux < > > [email protected] > > > > > > > > > > wrote: > > > > > > >>>>>>>>>> \o/ > > > > > > >>>>>>>>>> > > > > > > >>>>>>>>>> > > > > > > >>>>>>>>>> On Thu, Apr 24, 2014 at 2:30 PM, Anis KADRI < > > > > [email protected]> > > > > > > >>>>> wrote: > > > > > > >>>>>>>>>> > > > > > > >>>>>>>>>>> I just merged both browserify branches into master. > > There > > > > > > >> should > > > > > > >>>>> be no > > > > > > >>>>>>>>>>> impact. > > > > > > >>>>>>>>>>> Right now most specs pass expect for File, > > FileTransfer, > > > > > Media > > > > > > >> and > > > > > > >>>>>>>>> Contacts > > > > > > >>>>>>>>>>> due to some issues with merges/clobbers and I am > > looking > > > > into > > > > > > >>>>> those. > > > > > > >>>>>>>>>>> > > > > > > >>>>>>>>>>> Also, I got rid of the project cache condition in > > plugman > > > > > that > > > > > > >> was > > > > > > >>>>>>>>>>> preventing iOS frameworks from being added (CB-6441) > > > > > > >>>>>>>>>>> > > > > > > >>>>>>>>>>> Anis > > > > > > >> > > > > > > > > > > > > > > > > > > > > >
