Housekeeping updates

[taion]

Just cleaning up the config scripts a bit.

[taion]

90% of the reason for doing this is because right now Travis is showing:

because we have Travis builds enabled for non-master branches.

[taion]

And this won't disable Travis for v0.13.x, because Travis looks at the .travis.yml on the pushed branch, not the one on master.

[taion]

Wait - I'm definitely not okay with reverting this PR wholesale; it wasn't just the ES6 usage on the config files. The Karma setup made the debug info from normal test runs useless because it was instrumenting everything with the coverage data here. The corresponding reversion on the history side also took out all of the coverage reporting there.

I'm as much a fan of reasonable config as anyone, but nuking actual fixes for the sake of merging karma.conf.babel.js into karma.conf.js is not good!

[mjackson]

Can you make another PR that only includes the coverage fixes?

On Tue, Nov 10, 2015 at 5:08 PM Jimmy Jia notifications@github.com wrote:

Wait - I'm definitely not okay with reverting this PR wholesale; it wasn't
just the ES6 usage on the config files. The Karma setup made the debug
info from normal test runs useless because it was instrumenting everything
with the coverage data here. The corresponding reversion on the history
side also took out all of the coverage reporting there.

I'm as much a fan of reasonable config as anyone, but nuking actual fixes
for the sake of merging karma.conf.babel.js into karma.conf.js is not
good!

—
Reply to this email directly or view it on GitHub
#2492 (comment).

[mjackson]

In general we should keep PRs as small as possible. Fixes shouldn't be in a PR titled "housekeeping".

[taion]

I think we actively want ES6 for the Karma config for adding coverage. Specifically stuff like https://github.com/rackt/react-router/blob/v1.0.0/karma.conf.babel.js#L136 is a lot cleaner and easier to read than the ES5 equivalent.

I didn't cut it over to Babel just to replace require and var with import and const.

[mjackson]

Jimmy, you keep saying that. I'm saying that I don't want two files for
doing one thing. It's just syntax.

I appreciate the contributions you've made to the router, really I do. But
you don't need to fix stuff that isn't broken. In this case, it's just a
matter of preference.

On Tue, Nov 10, 2015 at 5:22 PM Jimmy Jia notifications@github.com wrote:

I think we actively want ES6 for the Karma config for adding coverage.
Specifically stuff like
https://github.com/rackt/react-router/blob/v1.0.0/karma.conf.babel.js#L136
is a lot cleaner and easier to read than the ES5 equivalent.

I didn't cut it over to Babel just to replace require and var with import
and const.

—
Reply to this email directly or view it on GitHub
#2492 (comment).

[taion]

I'm fine with that, and we can resolve those separately.

I don't have time to manually port the Karma config over from ES6 to ES5 right now. Are you okay with adding back the rest of the changes per my new PRs and treating moving the config code back to ES5 as a separate issue?

The PR actually did address real misconfigurations that I think we do care about, like the Karma debugger being unusable for the tests because of the instrumentation from isparta. I can take some time later to cut the configs back over to ES5, but reverting the entire PR throws out a lot more baby than bathwater.

[mjackson]

Part of the problem here is that when you changed the whole file it made it
really difficult for me to see which pieces were actually new stuff and
which pieces were just syntax/style. I spent the better part of a week a
few months back getting our testing infrastructure setup from end to end
(it's not easy, and there aren't a lot of great examples out there to
follow), and every decision that I made was intentional: the # of files,
their extensions, the syntax we used, etc.

Honestly, I'm fine with coverage being broken until we can get a PR that
only adds back just the coverage fixes. coverage isn't a bug in the code.
It's not essential. If there are other issues with debugging in Karma,
let's address those in a separate issue. But if we change a bunch of stuff
that wasn't broken in the same PR it makes it very difficult for me to
follow.

On Tue, Nov 10, 2015 at 5:32 PM Jimmy Jia notifications@github.com wrote:

I'm fine with that, and we can resolve those separately.

I don't have time to manually port the Karma config over from ES6 to ES5
right now. Are you okay with adding back the rest of the changes per my new
PRs and treating moving the config code back to ES5 as a separate issue?

The PR actually did address real misconfigurations that I think we do care
about, like the Karma debugger being unusable for the tests because of the
instrumentation from isparta. I can take some time later to cut the configs
back over to ES5, but reverting the entire PR throws out a lot more baby
than bathwater.

—
Reply to this email directly or view it on GitHub
#2492 (comment).

[taion]

I don't really care about whether the configs are in ES5 or ES6. Personally I feel a twinge of annoyance at not using template strings for patterns like https://github.com/rackt/react-router/blob/0fabc18fdfa317ef85f54ec4d7c086d9f6d38b3b/karma.conf.js#L109, but that's not important.

My issue is that I don't think it makes sense to directly back out a PR that was also signed off on by another member of the project - this wasn't a stack of commits I just pushed directly to the repo, and it had already gone through review.

I also intentionally submitted both this and the corresponding history PR as individual PRs with multiple commits because that seemed the best way to present this data - it's a number of small updates to config scripts, and would have just contributed to noise had they been submitted as separate PRs. Within the PR itself I've taken pains to make each of the commits as atomic and clear in its intention and action as possible.

I agree that coverage isn't the most important thing in the world, but e.g. on React Router, the Karma config to which you've reverted makes it very difficult to debug tests running locally because the tests are always run against code with coverage instrumentation. I also think that both coverage and being able to debug tests in Karma are worth more than ES5 v ES6 for config scripts.

I'm willing to take the time to move the configs back to ES5, but I feel that the reversion leaves both repos in a worse state in terms of code quality overall.