Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix Haxe path resolving from SDK #404

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Fix Haxe path resolving from SDK #404

wants to merge 1 commit into from

Conversation

as3boyan
Copy link
Contributor

@as3boyan as3boyan commented Feb 5, 2016

Fix Haxe path resolving from SDK

@as3boyan
Copy link
Contributor Author

as3boyan commented Feb 5, 2016

Discovered that is outputs mostly default "haxe"

and sometimes if Haxe SDK is not set up properly, like invalid file path, plugin would not check if file exists or not

@EBatTiVo
Copy link
Contributor

EBatTiVo commented Feb 5, 2016

So, why is it a bug in the plugin code if the user inputs an incorrect path? If we check this path at this point, and find that it doesn't exist, then we're going to try and run whatever the default haxe is, and not tell the user, which is a bad thing -- and can lead to subtle errors if the wrong compiler is used. So...

We should check whether the file exists when the user adds the command to the SDK (in the dialog code), and at this point, if there is a setting, but the file doesn't exist, we should put an error in the status bar at the bottom of the screen, put an error message into the system event log, and throw an error so that the completion continues to fail.

Is there a bug filed for this?

@EBatTiVo
Copy link
Contributor

EBatTiVo commented Feb 5, 2016

And, I guess that the subtle bugs are why you saw this as an issue in the first place. Maybe defaulting to 'haxe' is a bad thing to do in this function.

We should detect that at some other point -- maybe when we load the haxe settings for the first time: perhaps, HaxeSdkDta.loadState().

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants