fix #722 check path before passing it to basename

[muayyad-alsadi]

#722

[50Wliu]

@muayyad-alsadi Can you please add a spec for this? Thanks!

[muayyad-alsadi]

@50Wliu I'm not very familiar with your code structure

the affected function isCurrentFileBlackListed is called by findSuggestions which already have a spec in a file called spec/autocomplete-manager-integration-spec.coffee near line 1723 and 1742

https://github.com/atom/autocomplete-plus/blob/master/spec/autocomplete-manager-integration-spec.coffee#L1723

the problem with current specs that they open an existing file atom.workspace.open('sample.js') and the bug is cased by a new unsaved file (without path). which is supposed to already be tested by atom.workspace.open('') but I'm not sure why it did not trigger the error.

[the-question]

Just some thoughts:
Wouldn't it be better to consider the file as blacklisted (return true) if it's not saved.
It's difficult to know when a file is still empty (or nearly empty) what kind of content it will have and if the user will want to have autocomplete enabled on it. Moreover if the user kept the file as Plain Text, it may be hard to know how to autocomplete user's input.

Like @muayyad-alsadi, I'm not familiar with this plugin code, so I'm not sure that my comment makes any real sense.

My 2¢

[lee-dohm]

This is unnecessary when running Atom with a supported version of Electron. It is true that we'll have to address this eventually, but I think we'll address it in a more comprehensive way.

[muayyad-alsadi]

Atom with a supported version of Electron

what version is the supported version of Electron?

[50Wliu]

The version defined here (though note that's for master and not any released versions - you'll need to check the individual release branches for those).

It's currently 0.37.8 for Atom 1.9.0 and up and 0.36.8 for Atom 1.8.0.

[muayyad-alsadi]

so atom does not support 1.x release of electron?
please confirm this.

[the-question]

As I understand it, Atom isn't officially compatible with Electron 1.x, and as we can see, it's actually not fully compatible with it. But, as far as I can tell, it already works pretty well!
Maybe someone should open a new issue in Atom repository for that purpose (Electron 1.x compatibility).

[50Wliu]

so atom does not support 1.x release of electron?

As of this writing Atom does not support using anything other than the versions defined in its package.json. So no, Atom does not support using Electron 1.x.

@the-question Feel free to create a new issue.

[muayyad-alsadi]

understood.

@50Wliu is there any published reason for choosing an old legacy version of Electron? any link to any public discussion, ticket/issue, mailing list, ...etc.

was picking this specific old random version an aware decision or it just happened like that?

[muayyad-alsadi]

I opened an issue and here is the link

atom/atom#11967

[50Wliu]

0.37.8 is the last pre-1.0 version of Electron. We're not upgrading to 1.x yet as we need to give package authors some time to update and move away from deprecated APIs.

[muayyad-alsadi]

are we even sure it's about the version of Electron?

[50Wliu]

@muayyad-alsadi Can you clarify?

[muayyad-alsadi]

bug #722 is closed because it's reported on "unsupported version of Electron"
the trace seems to be a missing silly "if" statement before calling get basename of a file that can be undefined. I'm not sure how this can be a result of Electron's version.

I was told Electron new version removed deprecated APIs. I can't see a link between this #722 and Electron version. It was just a guess.

[50Wliu]

I'm not sure how this can be a result of Electron's version.

Newer versions of Electron also use newer versions of Node (specifically I believe v6 or so?). path is a Node API that added stricter checking to its passed-in arguments, leading to the error in #722.