Fix failed to load package

[jbazin30]

From this issue : #45

Based on this comment : atom/atom#15909 (comment)

This is my first pull request for an Atom package, so I could not test but when I tried it on package in Atom, it's work fine.

[smhxx]

Not actually sure why the CI is failing on this patch, atom --test spec seems to be exiting with 1 and no other output (which I'm able to repro on my local machine using jbazin30's code,) but when run from within Atom, all tests pass. Strange.

EDIT: Actually, now that I think about it, I think I might know why the CI is failing. I'm not sure atom respects atomTranspilers when running tests in headless mode, hence why the tests pass when run from within Atom but fail mysteriously when run with atom --test. I think this is something we've had to work around in the past (@GlenCFL may be able to confirm this, he knows more about it than I do,) although eventually I'll probably open a PR to fix this issue in Atom itself. (Never mind, it does.) For the time being, though, this patch actually does look OK to merge.

[GlenCFL]

@smhxx
I don't recall that. I think Atom does actually respect atomTranspilers when running atom --test, as I've leveraged this in a project recently. It could be an issue with the default Jasmine test runner, which I don't use.

I cant actually replicate the test failures using atom --test spec on this PR myself.

[smhxx]

Well, that's strange. Now that you mention it, I think you may be on to something with the default test runner thing. I remember there being some sort of issue with it, although it does strike me as odd that it would work when run from within Atom but not when run from the command line... I'll have to acquaint myself with the internals of the Jasmine test runner and see where the issue stems from. (For personal reference: https://github.com/atom/atom/blob/fc53b2185716174dc700a963e8fb73e68a921449/spec/jasmine-test-runner.coffee)

[smhxx]

Looking back at smhxx/atom-ts-transpiler#8, I'm now realizing that the issue I was thinking of was with the Jasmine test runner not running tests written in TypeScript. So not really the same thing we're seeing here, where the tests are JS but the code is TS.

EDIT: Just to jump in and clarify the situation for posterity's sake, the only reason the Jasmine runner doesn't run tests written in TypeScript is the file extension; the transpilation isn't actually an issue. Atom will invoke the transpiler just fine when requiring your spec files, but the test runner is also responsible for indicating which files to treat as part of the test suite, and it doesn't currently look for files with a .ts extension.

[cdbattags]

+1

What can I do to help to fix this? Have we determined it's the transpiler?

[smhxx]

Actually, I'm pretty sure it can't be the transpiler... not least of all because the only file that's actually required by the test suite is written in CoffeeScript, which Atom understands natively. I'm not fully convinced the CI failure is even due to the changes in the PR, to be honest. It could simply be that Atom's CI script (or Atom itself) was changed somehow since the last time anyone pushed to master, and the tests error out regardless of jbazin30's changes...

[cdbattags]

Regardless of the CI not working correctly, I am not able to use the plugin in its current form and I'm receiving the same error. I would love the ability to pretty quickly revert line by line changes again.

[smhxx]

The code in this PR does work, I've tested it personally and it does restore the normal functionality of the package. If you're looking to enable line-diff-details ASAP without waiting for the PR to be merged, you can clone directly from jbazin30/line-diff-details, then from the line-diff-details directory:

npm install --only=production
apm link

or simply clone it directly into your ~/.atom/packages/line-diff-details directory and then npm install to grab the dependencies.

[smhxx]

Strangely, I just cloned both forks to see whether the test failures were actually related to the changes in the PR, and I'm no longer able to repro the error that I was previously getting from atom --test. Perhaps this was an unrelated bug with Atom that was causing the tests to not be run properly? I'd be interested to see whether the build passes now if the job is restarted on Travis. /cc @jakesankey

[smhxx]

Huh, interesting. Seems to still be failing on Travis, but now I'm unable to replicate the failure locally (even though I was able to before.) Very strange.

[jakesankey]

Sorry I haven't been able to help out on this one. I am not actively developing these plugins at the moment. I'm glad you all like the functionality of this one. If you feel it is working properly, I can merge it for you and publish a patch release later today if I get a chance :)

[smhxx]

Understandable. My personal intrigue mainly stems from the fact that I'm the creator of atom-ts-transpiler, so if my package is somehow causing these strange build failures, I'd like to isolate the issue so that I can correct it (although, like I said earlier, I'd be surprised if it were actually to blame given the nature of the error.) I'm making one last effort to figure out what's causing this oddity; if I'm not able to, perhaps merging regardless of the build status might be an acceptable path forward.

[smhxx]

Well, now I feel dumb. Took me about 30 seconds to realize what's going on after actually taking a serious look at the changes. Atom's tripping up because you have atom-babel6-transpiler listed as a transpilation handler, but it's not in the package's dependencies, so trying to register the transpiler fails (meaning apm test exits with code 1 before ever trying to run the specs.)

In fact, I would just remove this entry altogether, since having a custom transpiler for .js files isn't actually necessary here, and the glob doesn't match any files, anyway. I might also point out that this section is using 4-space indentation whereas the rest of the file is using 2-space indentation, although that's less of a serious issue and more of a style thing.