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

feat: added support for NixOS config files #229

Merged
merged 3 commits into from
May 31, 2024

Conversation

KhanKudo
Copy link
Contributor

@KhanKudo KhanKudo commented May 9, 2024

This PR resolves the feature request from the official vscode-nix-ide extension repository. (Closes nix-community/vscode-nix-ide#379)
It doesn't change any of the existing behavior, only extends it by adding a new provider for **/*.nix file pattern. This provider has also been placed after the javascript provider to ensure it never overwrites it.
The primary complexity of implementing this feature was the fact that in .nix configs file paths are primarily placed outside of "quotes" resulting in the default vscode context.fromString property being empty, as there is no string to recognize. A custom short regex has been used to match the expected behavior of context.fromString in cases without quotation marks. Worth noting is that, if the path is placed in a string, the default result of context.fromString is forwarded, without invoking any of the custom regex recognition.

@KhanKudo KhanKudo changed the title Added support for NixOS .nix config files feat: added support for NixOS config files May 10, 2024
@ChristianKohler
Copy link
Owner

Hi @KhanKudo

thank you for your contribution. First look looks good. Nicely isolated. I will try to give a better look in the next week.

@KhanKudo
Copy link
Contributor Author

Awesome, glad to hear you like it. It would be fantastic to close the feature request. This behavior has been wished for since at least January. Given the large user base of Path-Intellisense, I tried my best to make sure there were no unintended consequences, thus the selector limits use to explicitly .nix files. The implementation however could very well be useful in .sh or other text files where paths are commonly used outside of quoted strings, such as for GitLab/GitHub CI/CD configs.

@ChristianKohler
Copy link
Owner

Hi @KhanKudo

I reviewed and tested your PR. It works as expected and the code is easy to understand. I like how you added it as a separate provider and made sure it doesn't conflict with the existing solution. Thank you for your amazing work 🤩

I noticed that you are reusing the provide from the JavaScript provider. It is almost the same code so I see why it would make sense. My idea with the providers and the util functions was, that the common code is in utils and the providers do not depend on each other. This ensures that single providers can be refactored in the future without me having to worry that I break another provider.

Could you duplicate code? It is a few duplicates line but mostly just calls to utils fns. The file createCompletionItem.ts which is currently in the Javascript provider could be moved into the utils folder. It is not 100% provider agnostic but enough to justify to share it. This way, the providers are completely independent from each other. Would that be good?

Note: the test are not running on Ci. This has nothing to do with your change. I have to check what the issue.

Thank you again for your contribution. Looking forward to merge the first provider next to JS

@KhanKudo
Copy link
Contributor Author

I absolutely agree, different providers should not depend on each other. I have pushed a new commit that moves createCompletionItem.ts and createContext.ts to the utils folder. And whilst I usually prefer to avoid code duplication at all cost, I definitely have to agree that in this case, it is a lot better to have some duplication but keep independent code separate.
Given that javascript and nix have basically nothing in common, it is certainly a good practice to assume they might have different needs and thus provider differences in the future.

@ChristianKohler
Copy link
Owner

Thank you @KhanKudo for making those changes. Looks very good.

The only thing you missed is to revert the changes in the javascript provider in the provide function. We don't need to export it anymore and have the optional parameter. Could you fix that? After that from my side the Nix support is ready to be published :-)

@KhanKudo
Copy link
Contributor Author

Oh that's a silly oversight haha.
I'll fix it in just a sec ;)

@ChristianKohler
Copy link
Owner

Thank you @KhanKudo for your contribution. Nice work and pleasant discussion. I will create a new release with your PR. 🚀

@ChristianKohler ChristianKohler merged commit 6aa458c into ChristianKohler:master May 31, 2024
github-actions bot pushed a commit that referenced this pull request Jun 1, 2024
# [2.9.0](v2.8.0...v2.9.0) (2024-06-01)

### Features

* added support for NixOS config files ([#229](#229)) ([6aa458c](6aa458c))
@KhanKudo KhanKudo deleted the nixos-config-support branch June 7, 2024 09:32
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.

Feature: Path intellisense
2 participants