miqh / atom-rustsym

Provides symbol navigation for Rust code in Atom
MIT License
1 stars 1 forks source link

Executable Path in config is ignored. #1

Closed exoticorn closed 7 years ago

exoticorn commented 7 years ago

This code either errors out or sets the command to "rustsym", so the executable path is solely use to check whether the executable exists and then ignored after that:

    if (command && !fs.existsSync(command)) {
      return Promise.reject({
        message: 'Please check your \`rustsym\` executable path setting.',
        stack: `The executable path provided (\`${command}\`) does not exist on the file system.`
      });
    } else {
      command = 'rustsym';
    }
ghost commented 7 years ago

@exoticorn, are there any UI errors you're seeing when using the plugin that prompted you to point out this code snippet?

Every symbol search should attempt to read the executable path setting (i.e. the change will take effect on the subsequent searches). If none is specified, it assumes rustsym is available via PATH and thus sets the command to invoke accordingly.

One thing I've noticed with the Atom settings pane (at least on my Windows machine) is that filling out a text input then defocusing by clicking directly into another pane will not save the contents just entered (nb. hitting ENTER appears to trigger the save). Perhaps you're setting a custom executable path, but it's not being saved by the settings?

exoticorn commented 7 years ago

I had this issue at work, so I can't be 100% sure, but as far as I remember, the only visible 'error' was that I didn't get displayed any symbols. I then stepped through the code to see where the problem was, noticed this 'if', fixed the code locally and verified that I then got a correct list of symbols.

Look at the code again, it says: IF executable path is set in the settings AND that path doesn't exist in the fs, return an error. IN ALL OTHER CASES (ie. all non-error cases): set the command to 'rustsym' (ie. try to run rustsym from the PATH.) Specifically, if the executable path is set and the file exists, this package tries to run rustsym from PATH, anyway.

The code should probably read like this instead:

if(command) {
  if(!fs.existsSync(command)) {
    return error;
  }
} else {
  command = 'rustsym';
}
ghost commented 7 years ago

Ah, right you are. Thank you for elaborating—I feel a little silly not having seen the obvious. :bow:

Would you like to make a pull request to implement this small fix? Otherwise I can patch it up.

exoticorn commented 7 years ago

No Problem. :)

I can send you a pull request on Monday, provided I don't forget until then.