-
Notifications
You must be signed in to change notification settings - Fork 179
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
Allow configuring extra chruby paths #1976
Conversation
Is it not possible to just pull this from the environment variable? That seems like a cleaner solution, if possible. |
Not in a scalable and clean way. For example:
Trying to integrate with shells (our previous approach) basically resulted in a variety of problems. People can configure their environments in unexpected ways that don't integrate well with the extension. As examples, we had reports of users with shell plugins that refused to activate if they weren't in a TTY, breaking our integration. In other cases, shell plugins hijacked the stderr pipe, redirecting it to something else and only restoring the original file descriptor if the user was about to write a command, which never gets detected when we invoke the shell from NodeJS - again breaking the editor/server communication. In yet another case, some shell plugin maxed the user's CPU when we invoke their shell from the NodeJS process. Probably because it did not account for the case of having the shell be invoked as a script. Basically, integrating with shells proved to be extremely brittle and lead to a lot of frustration. We invested significant effort to decouple from shells to prevent all of those. I understand that having to set |
@vinistock thanks for all the detail! This seems like a reasonable tradeoff. |
a5ad0d0
to
c447b1c
Compare
c447b1c
to
88631cc
Compare
Motivation
Addressed the first part of #1969
Chruby allows configuring extra directories to search for Ruby installations. We'll need to offer a similar setting to match the behaviour.
Implementation
Added the setting and started pushing the configured URIs into the list of places to search for.
Automated Tests
Added a test.