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

Updated message for missing dependencies #123

Merged
merged 8 commits into from
Jul 24, 2023
Merged

Conversation

petervdonovan
Copy link
Contributor

This is mostly relevant to packages managed by NVM, which seem to be located in a special place.

Fixes #119. This does not actually fix the PATH, but it does at least update the error message so that the message suggests the correct solution and is not misleading.

I believe that we can fix the PATH if we really want to by doing some hacking with the embedded terminal. However, I anticipate that such a solution would cause bugs and maintainability problems that I do not want to face right now.

This is mostly relevant to packages managed by NVM, which seem to be
located in a special place.
@petervdonovan
Copy link
Contributor Author

It was my mistake to open this PR without waiting for CI to pass. I think it is truly ready to merge now.

@@ -62,7 +60,7 @@ jobs:
platform: [ubuntu-latest, macos-latest, windows-latest]
runs-on: ${{ matrix.platform }}
steps:
- uses: lf-lang/vscode-lingua-franca/.github/actions/build@main
- uses: lf-lang/vscode-lingua-franca/.github/actions/build@update-error-message
Copy link
Member

@lhstrh lhstrh Jun 30, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we get rid of the @ref by referencing the workflow locally, like we do in lingua-franca now?

Copy link
Member

@lhstrh lhstrh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good modulo comment...

@@ -49,7 +44,10 @@ jobs:
platform: [ubuntu-latest, macos-latest, windows-latest]
runs-on: ${{ matrix.platform }}
steps:
- uses: lf-lang/vscode-lingua-franca/.github/actions/build@main
- uses: lf-lang/vscode-lingua-franca/.github/actions/build@update-error-message
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we check out the repo in this workflow instead of build.yml, I think we could refer to it with a local path and won't need the @.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tried that and it did not work conveniently, but I do not remember why. It could just be that I would need to spend a bit of time rearranging some YAML, but I have not found the time to do that yet.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, then let's not waste time on it. It's not a big deal. Let's merge, adjust the refs, and only then delete this branch.

@petervdonovan petervdonovan merged commit 1f33508 into main Jul 24, 2023
10 checks passed
@petervdonovan petervdonovan deleted the update-error-message branch July 24, 2023 05:17
@lhstrh lhstrh changed the title Update message for missing dependencies Updated message for missing dependencies Aug 28, 2023
@lhstrh lhstrh added the enhancement New feature or request label Aug 28, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

VS Code incorrectly detects missing dependencies
2 participants