Fixes the exception thrown when the provider list for a given language is undefined.

[Mark-Simulacrum]

My fault for not noticing this before. Hope that no one is majorly affected by this error!

/cc @MiguelCastillo

[MiguelCastillo]

No problem. We should probably add a unit test for that and other similar smallish code paths

[Mark-Simulacrum]

I'm surprised that this unit test didn't catch this bug -- and I'm not sure about any other code paths that could be tested.

Should I add a unit test that modifiying the returned array doesn't modify CodeInspection's version of the array?

[busykai]

Well, I actually noticed it, but it was too late. :) .slice(0) would throw. I've actually fixed it in #7362, here.

[MiguelCastillo]

The unit test I was thinking about is to go back to the crash, add a test that reproduces the issue, then fix it so that the unit test passes.

[busykai]

There are 3 failing tests due to this in master. No need to add additional ones.

@Mark-Simulacrum, it's necessary to reload/restart Brackets before running the tests again. Otherwise you run old code (which didn't fail because it didn't have .slice(0))

EDIT: or better yet -- use Debug -> New Brackets Window and run the tests from it.

[Mark-Simulacrum]

Ah, that must have been my mistake -- I opened and closed the test window, but didn't reload Brackets.

[busykai]

Could you actually split this into two steps? Something like

var providers = _providers[LanguagerManager...];
return (providers && providers.slice(0)) || [];

It's not easily readable...

[Mark-Simulacrum]

Fixed.

[MiguelCastillo]

@busykai Cool. If we have failing tests than that means it was just an oversight rather than lack of unit tests. So no need to add a unit test.

[Mark-Simulacrum]

There is still more to be done. Since slice(0) doesn't perform a deep copy, the objects are not duplicated. I'm currently looking for the best way to perform a deep copy.

[busykai]

there's a _.cloneDeep, but i don't think it's needed. modifying a provider is a malicious action. people will not write an extensions like that in good faith.

[MiguelCastillo]

@Mark-Simulacrum you run a providers.map(function(item) { return _.cloneDepp(item); }) type of step

[MiguelCastillo]

@busykai I agree with you... immutable objects would be nice

[Mark-Simulacrum]

@MiguelCastillo @busykai You suggested adding the slice(0) to the function here: #9189 (comment). I'm confused as to whether we need/want this change.

[MiguelCastillo]

@Mark-Simulacrum slice is good, that stays. We don't want people being able to modify the array itself; add/remove items from it.

I am torn about the deep clone though. We want to keep people from unintentionally messing up the providers.

[Mark-Simulacrum]

If we do a deep copy, it is somewhat difficult to check within a unit test environment if the linter(s) returned from the function are the linters that we registered, because they are cloned and as such are not strictly equal to each other.

[busykai]

I think freezing providers on registration is a good idea, but it's out of scope of this PR. This one fixes much more immediate issue and needs to be merged ASAP. :)

[MiguelCastillo]

@busykai agreed. Merging.

[ingorichter]

Sorry, I have missed this. Thanks for fixing to quickly.