Skip to content

nixd/tools/nixd-attrset-eval: support completion for "builtins"#620

Closed
inclyc wants to merge 5 commits into
mainfrom
support-builtins
Closed

nixd/tools/nixd-attrset-eval: support completion for "builtins"#620
inclyc wants to merge 5 commits into
mainfrom
support-builtins

Conversation

@inclyc

@inclyc inclyc commented Nov 19, 2024

Copy link
Copy Markdown
Member

Basic support for option completion on the worker process. This is the premise for doing this in LSP.

Link: #507

@inclyc inclyc requested a review from Mic92 November 19, 2024 08:45
Comment thread nixd/tools/nixd-attrset-eval/test/builtin-info.md Outdated
Comment thread nixd/tools/nixd-attrset-eval/test/builtin-info.md Outdated
Comment thread nixd/tools/nixd-attrset-eval/test/builtin-completion.md Outdated
Comment thread nixd/tools/nixd-attrset-eval/test/builtin-completion.md Outdated
Comment thread nixd/include/nixd/Protocol/AttrSet.h Outdated
@Mic92

Mic92 commented Nov 19, 2024

Copy link
Copy Markdown
Member

@inclyc did you request my review for something specific or more for a general review?

Comment thread nixd/lib/Eval/AttrSetProvider.cpp Outdated
@inclyc

inclyc commented Nov 19, 2024

Copy link
Copy Markdown
Member Author

@inclyc did you request my review for something specific or more for a general review?

A specific review for C++ nix IPC:

This pull request introduces the ability to query builtins information. Do you think the methodology is correct, or does it need refinement?

Very appreciated if you can take a look :)

@Mic92

Mic92 commented Nov 19, 2024

Copy link
Copy Markdown
Member

It looks reasonable to me and similar to what the nix repl does for completing. However I would prefer if @roberth would have a look as well.

Comment thread libnixt/include/nixt/Value.h

/// \brief Get nix's `builtins` constant
inline nix::Value &getBuiltins(const nix::EvalState &State) {
return *State.baseEnv.values[0];

@Mic92 Mic92 Nov 20, 2024

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I would do add a TODO now that this could use the proper api in the next nix version.

@inclyc inclyc marked this pull request as draft November 21, 2024 03:14
@PriceHiller

Copy link
Copy Markdown

Hey! Checking in here.

What all needs to be done to get this working?

More than happy to get the ball rolling assuming I can be of any help.

@inclyc

inclyc commented Feb 17, 2025

Copy link
Copy Markdown
Member Author

Hey! Checking in here.

What all needs to be done to get this working?

More than happy to get the ball rolling assuming I can be of any help.

Actually, this PR is related to the technical approach of #625. I’m considering proposing a technical solution that would work well for both #625 and this PR, but I haven’t had the time to write the code recently. If you’re interested, you could explore the connection between the technical approaches of these two PRs and help move this work forward.

inclyc and others added 5 commits April 26, 2025 23:55
Basic support for option completion on the worker process.
This is the premise for doing this in LSP.
Co-authored-by: Jörg Thalheim <Mic92@users.noreply.github.com>
@inclyc inclyc force-pushed the support-builtins branch from 24792c1 to af12b9c Compare April 26, 2025 15:56
@inclyc

inclyc commented Apr 26, 2025

Copy link
Copy Markdown
Member Author

Rebased. Resolving merge conflicts.

@inclyc inclyc closed this May 4, 2025
@inclyc

inclyc commented May 4, 2025

Copy link
Copy Markdown
Member Author

Closed, prefer #677

@inclyc inclyc deleted the support-builtins branch May 4, 2025 14:51
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.

4 participants