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 resolve of protolint.path if configured relative #36

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

Conversation

yrtimiD
Copy link

@yrtimiD yrtimiD commented Aug 1, 2023

Fixes relative path usage which was resolved to the user folder (~), (very unexpected behavior)

This fix allows 3 cases for protolint.path config:

  • empty or default protolint (used as-is)
  • absolute path (used as-is)
  • relative (resolved relatively to the first workspace folder)

Added settings caching.
Added settings reload only if changed.

@AlexCannonball
Copy link

@yrtimiD Hello, could you describe a scenario where the relative path makes sense? Maybe accepting just an absolute path is better?

@yrtimiD
Copy link
Author

yrtimiD commented Oct 1, 2023

In my specific case I wanted to keep protolint binary committed to the git, so its location will always be relative to the vscode workspace folder. This is not super right, but saves from everybody extra step of download and install the binary. Alternatively, configuration might allow using vscode's variables like ${workspaceRoot}

@RobertAron
Copy link

The relative path would be extremely useful if you have protolint installed via npm.

In my case I tried to...

  • run npm install protolint
  • in .vscode/extensions.json set "recommendations":["plex.vscode-protolint"]
  • in .vscode/settings.json set "protolint.path": "./node_modules/.bin/protolint"

This way anyone on my team who runs npm install on the project will get linting on the file. The current workaround is to have information in the readme that explains to my team they need to brew install protolint which is not as good as an experience.

@RobertAron
Copy link

I would really appreciate if these changes were considered 🙏

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.

3 participants