search

castwide / vscode-solargraph

A Visual Studio Code extension for Solargraph.
Other
425 stars 23 forks source link

do not set solargraph.bundlerPath as executable #31

Closed g-arjones closed 6 years ago

g-arjones commented 6 years ago

This allows the user to set the property on a per-workspace level

castwide commented 6 years ago

I've been on the fence about the isExecutable flag for solargraph.bundlerPath and solargraph.commandPath. On one hand, it would certainly be useful for people to be able to set the paths for specific workspaces. On the other hand, the flag exists so settings in a shared code base (e.g., a repo that includes a .vscode/settings.json file) won't override the user's preferences. I can see how users might not want projects to determine their executable paths for them.

I'd like to get some more feedback from users to determine whether 1) the benefit of removing the isExecutable flag outweighs the risks, and 2) there might be other ways to solve the problem of setting paths per workspace.

g-arjones commented 6 years ago

Setting the bundlerPath in the user settings doesn't make much sense, IMHO. The whole point of bundler is to have separate environments/workspaces for different projects. Disallowing the setting in the workspace kinda defeats its own purpose I guess..

castwide commented 6 years ago

The more I think about it, the more I'm tempted to agree. Any other solution would have the exact same issue with executable paths being set in shared repos, so all it would do is move the risk from one place to another.

I expect to merge this change and do the same with solargraph.commandPath.

g-arjones commented 6 years ago

Would you like me to do it here or would you rather do it in another PR to keep things separate?

castwide commented 6 years ago

If you want to take care of it, I think it makes sense for both changes to be part of the same PR. Thanks.

g-arjones commented 6 years ago

No problem..

g-arjones commented 6 years ago

Done.