You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@cordova.apache.org by Jesse <pu...@gmail.com> on 2014/08/20 00:59:59 UTC
continuous integration woze
While I think it is a great practice to run hinting/linting tools,
according to Travis, this pull request should be rejected. [1] The
offending line [2]
src/cordova/lazy_load.js: line 152, col 26, Missing semicolon.
Is there anyway we can make this more of a warning and less of a drop
everything and fix the build?
[1] https://github.com/apache/cordova-lib/pull/75
[2]
https://github.com/blackberry-webworks/cordova-lib/blob/cb_7336/cordova-lib/src/cordova/lazy_load.js#L152
@purplecabbage
risingj.com
Re: continuous integration woze
Posted by Mark Koudritsky <ka...@google.com>.
+1 for what Michal wrote (including the self bikeshedding :)
It's a matter of JSHint config and I'm ok with changing it.
Currently the JSHint config in cordova-lib is per file (using /* jshint */
config comments) and slightly differs between the files to accommodate for
different styles present in the project.
On Fri, Aug 22, 2014 at 3:14 PM, Michal Mocny <mm...@chromium.org> wrote:
> Sounds like we should leave the PR checks and testing as is: if it fails
> jshint, it "breaks the build".
>
> What we can re-evaluate is our .jshintrc defaults: do we want to allow
> missing ; in our codebase? Seems like this is a very divided issue. I'm
> pro semicolons, but I hear thats old-school. I'm also pro-choice ;) so I'd
> be happy to remove that option. I'm also pro-consistency so I'd rather we
> use semicolons everywhere or nowhere. Sounds like I'm bikeshedding with
> myself!
>
> -Michal
>
>
> On Fri, Aug 22, 2014 at 2:42 PM, Jesse <pu...@gmail.com> wrote:
>
> > okay, that is what I did ...
> > I will leave things as is, I just wanted to judge if everyone felt that 1
> > missing (though not required) ';' is enough to break the build.
> >
> > @purplecabbage
> > risingj.com
> >
> >
> > On Fri, Aug 22, 2014 at 9:40 AM, Lorin Beer <lo...@gmail.com>
> wrote:
> >
> > > +1 to fixing the test
> > > -1 to warnings
> > >
> > > warnings are either treated like errors, or ignored entirely. I'm for
> > > treating warnings like errors in the first place
> > >
> > >
> > > On Thu, Aug 21, 2014 at 1:53 PM, Marcel Kinard <cm...@gmail.com>
> > wrote:
> > >
> > > > I agree. My personal philosophy is generally to treat warnings as
> > errors.
> > > >
> > > > On Aug 21, 2014, at 9:27 AM, Mark Koudritsky <ka...@google.com>
> > wrote:
> > > >
> > > > > If some tests are too strict or too flaky, we can and should fix or
> > > > remove
> > > > > them altogether. I don't believe there is any value in warnings,
> they
> > > > will
> > > > > accumulate and then people will largely ignore them.
> > > >
> > > >
> > >
> >
>
Re: continuous integration woze
Posted by Jesse <pu...@gmail.com>.
I am pro consistency as well, and I prefer including semi-colons shows your
intent, where a missing semi-colon could be intentional, or just forgotten.
My ultimate point with this thread was just that I feel the CI tests should
be concerned with function, and not form. I honestly don't care if people
use or don't use semi-colons, spaces, tabs, trailing spaces, cuddled
braces, ... as long as the code is readable, understandable, and
demonstrates intent.
Anyway, it is all fine, we can move on.
@purplecabbage
risingj.com
On Fri, Aug 22, 2014 at 12:17 PM, Josh Soref <js...@blackberry.com> wrote:
> Michal Mocny wrote:
> >What we can re-evaluate is our .jshintrc defaults: do we want to allow
> >missing ; in our codebase? Seems like this is a very divided issue. I'm
> >pro semicolons, but I hear thats old-school. I'm also pro-choice ;) so
> >I'd
> >be happy to remove that option.
>
> >I'm also pro-consistency so I'd rather we use semicolons everywhere or
> >nowhere.
>
> +1
>
>
Re: continuous integration woze
Posted by Josh Soref <js...@blackberry.com>.
Michal Mocny wrote:
>What we can re-evaluate is our .jshintrc defaults: do we want to allow
>missing ; in our codebase? Seems like this is a very divided issue. I'm
>pro semicolons, but I hear thats old-school. I'm also pro-choice ;) so
>I'd
>be happy to remove that option.
>I'm also pro-consistency so I'd rather we use semicolons everywhere or
>nowhere.
+1
Re: continuous integration woze
Posted by Michal Mocny <mm...@chromium.org>.
Sounds like we should leave the PR checks and testing as is: if it fails
jshint, it "breaks the build".
What we can re-evaluate is our .jshintrc defaults: do we want to allow
missing ; in our codebase? Seems like this is a very divided issue. I'm
pro semicolons, but I hear thats old-school. I'm also pro-choice ;) so I'd
be happy to remove that option. I'm also pro-consistency so I'd rather we
use semicolons everywhere or nowhere. Sounds like I'm bikeshedding with
myself!
-Michal
On Fri, Aug 22, 2014 at 2:42 PM, Jesse <pu...@gmail.com> wrote:
> okay, that is what I did ...
> I will leave things as is, I just wanted to judge if everyone felt that 1
> missing (though not required) ';' is enough to break the build.
>
> @purplecabbage
> risingj.com
>
>
> On Fri, Aug 22, 2014 at 9:40 AM, Lorin Beer <lo...@gmail.com> wrote:
>
> > +1 to fixing the test
> > -1 to warnings
> >
> > warnings are either treated like errors, or ignored entirely. I'm for
> > treating warnings like errors in the first place
> >
> >
> > On Thu, Aug 21, 2014 at 1:53 PM, Marcel Kinard <cm...@gmail.com>
> wrote:
> >
> > > I agree. My personal philosophy is generally to treat warnings as
> errors.
> > >
> > > On Aug 21, 2014, at 9:27 AM, Mark Koudritsky <ka...@google.com>
> wrote:
> > >
> > > > If some tests are too strict or too flaky, we can and should fix or
> > > remove
> > > > them altogether. I don't believe there is any value in warnings, they
> > > will
> > > > accumulate and then people will largely ignore them.
> > >
> > >
> >
>
Re: continuous integration woze
Posted by Jesse <pu...@gmail.com>.
okay, that is what I did ...
I will leave things as is, I just wanted to judge if everyone felt that 1
missing (though not required) ';' is enough to break the build.
@purplecabbage
risingj.com
On Fri, Aug 22, 2014 at 9:40 AM, Lorin Beer <lo...@gmail.com> wrote:
> +1 to fixing the test
> -1 to warnings
>
> warnings are either treated like errors, or ignored entirely. I'm for
> treating warnings like errors in the first place
>
>
> On Thu, Aug 21, 2014 at 1:53 PM, Marcel Kinard <cm...@gmail.com> wrote:
>
> > I agree. My personal philosophy is generally to treat warnings as errors.
> >
> > On Aug 21, 2014, at 9:27 AM, Mark Koudritsky <ka...@google.com> wrote:
> >
> > > If some tests are too strict or too flaky, we can and should fix or
> > remove
> > > them altogether. I don't believe there is any value in warnings, they
> > will
> > > accumulate and then people will largely ignore them.
> >
> >
>
Re: continuous integration woze
Posted by Lorin Beer <lo...@gmail.com>.
+1 to fixing the test
-1 to warnings
warnings are either treated like errors, or ignored entirely. I'm for
treating warnings like errors in the first place
On Thu, Aug 21, 2014 at 1:53 PM, Marcel Kinard <cm...@gmail.com> wrote:
> I agree. My personal philosophy is generally to treat warnings as errors.
>
> On Aug 21, 2014, at 9:27 AM, Mark Koudritsky <ka...@google.com> wrote:
>
> > If some tests are too strict or too flaky, we can and should fix or
> remove
> > them altogether. I don't believe there is any value in warnings, they
> will
> > accumulate and then people will largely ignore them.
>
>
Re: continuous integration woze
Posted by Marcel Kinard <cm...@gmail.com>.
I agree. My personal philosophy is generally to treat warnings as errors.
On Aug 21, 2014, at 9:27 AM, Mark Koudritsky <ka...@google.com> wrote:
> If some tests are too strict or too flaky, we can and should fix or remove
> them altogether. I don't believe there is any value in warnings, they will
> accumulate and then people will largely ignore them.
Re: continuous integration woze
Posted by Carlos Santana <cs...@gmail.com>.
+1 just fix the code to pass the test, or fix the test to be more accurate.
-1 warnings
On Thu, Aug 21, 2014 at 9:27 AM, Mark Koudritsky <ka...@google.com> wrote:
> If some tests are too strict or too flaky, we can and should fix or remove
> them altogether. I don't believe there is any value in warnings, they will
> accumulate and then people will largely ignore them.
>
> There are also tests running or real devices here: http://ci.cordova.io/
>
--
Carlos Santana
<cs...@gmail.com>
Re: continuous integration woze
Posted by Mark Koudritsky <ka...@google.com>.
If some tests are too strict or too flaky, we can and should fix or remove
them altogether. I don't believe there is any value in warnings, they will
accumulate and then people will largely ignore them.
There are also tests running or real devices here: http://ci.cordova.io/
RE: continuous integration woze
Posted by "Parashuram Narasimhan (MS OPEN TECH)" <pa...@microsoft.com>.
If some of the tests are too strict, should we move it to be triggered on a different branch? For example, fails come only on master, while they show up as "warning" for other branches and we build --force incase of other branches?
-----Original Message-----
From: Josh Soref [mailto:jsoref@blackberry.com]
Sent: Wednesday, August 20, 2014 8:00 AM
To: dev@cordova.apache.org
Subject: Re: continuous integration woze
On 8/20/14, 10:50 AM, "Mark Koudritsky" <ka...@google.com> wrote:
>AFAIK Travis only supplies a binary pass/fail status, so there is no
>such thing as a warning.
>A Travis check for pull request doesn't mark the entire build on master
>as failed, only that pull request. So there is no need to panic and
>"drop everything and fix the build", instead just "fix the build" by
>updating the pull request :)
Fwiw, I thought that I had pushed an update to the pull request, but apparently my `git commit —amend` was missing a `-a`, and thus my `git push -f` didn't do anything useful.
>That said, I would be glad if Travis was commenting on pull request
>with a line or two rather than just setting the binary pass/fail
>status. This would also be a great feature for using multiple CI
>services like we do with Travis and AppVeyor. Anybody knows if Travis
>can be configured to leave comments under pull requests?
I would much prefer this, because as is, I have at least 4 "outputs" from
travis+appveyor,
and yet if you look at the pull request, you'll only see one.
Re: continuous integration woze
Posted by Josh Soref <js...@blackberry.com>.
On 8/20/14, 10:50 AM, "Mark Koudritsky" <ka...@google.com> wrote:
>AFAIK Travis only supplies a binary pass/fail status, so there is no such
>thing as a warning.
>A Travis check for pull request doesn't mark the entire build on master as
>failed, only that pull request. So there is no need to panic and "drop
>everything and fix the build", instead just "fix the build" by updating
>the
>pull request :)
Fwiw, I thought that I had pushed an update to the pull request,
but apparently my `git commit —amend` was missing a `-a`,
and thus my `git push -f` didn't do anything useful.
>That said, I would be glad if Travis was commenting on pull request with a
>line or two rather than just setting the binary pass/fail status. This
>would also be a great feature for using multiple CI services like we do
>with Travis and AppVeyor. Anybody knows if Travis can be configured to
>leave comments under pull requests?
I would much prefer this, because as is, I have at least 4 "outputs" from
travis+appveyor,
and yet if you look at the pull request, you'll only see one.
Re: continuous integration woze
Posted by Mark Koudritsky <ka...@google.com>.
AFAIK Travis only supplies a binary pass/fail status, so there is no such
thing as a warning.
A Travis check for pull request doesn't mark the entire build on master as
failed, only that pull request. So there is no need to panic and "drop
everything and fix the build", instead just "fix the build" by updating the
pull request :)
Those things are painful at first, but pain is an excellent teacher when it
comes to acquiring good habits like running "npm test" before git push. So
things like forgotten semicolons quickly become a non-issue once you hit
them several times.
That said, I would be glad if Travis was commenting on pull request with a
line or two rather than just setting the binary pass/fail status. This
would also be a great feature for using multiple CI services like we do
with Travis and AppVeyor. Anybody knows if Travis can be configured to
leave comments under pull requests?
On Tue, Aug 19, 2014 at 6:59 PM, Jesse <pu...@gmail.com> wrote:
> While I think it is a great practice to run hinting/linting tools,
> according to Travis, this pull request should be rejected. [1] The
> offending line [2]
>
> src/cordova/lazy_load.js: line 152, col 26, Missing semicolon.
>
> Is there anyway we can make this more of a warning and less of a drop
> everything and fix the build?
>
> [1] https://github.com/apache/cordova-lib/pull/75
> [2]
>
> https://github.com/blackberry-webworks/cordova-lib/blob/cb_7336/cordova-lib/src/cordova/lazy_load.js#L152
>
> @purplecabbage
> risingj.com
>