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

Move completion code into its own crate. #2591

Closed
wants to merge 1 commit into from
Closed

Move completion code into its own crate. #2591

wants to merge 1 commit into from

Conversation

thegedge
Copy link
Contributor

This allows the completions crate to be completely decoupled from nu-cli, allowing it to be developed independently from most other nu crates.

The major change in this commit is making the completion context be a trait. Currently limited to returning a signature registry, we'll likely have to expand this as time goes on.

Other approaches

Another approach would be to eliminate the context completely, and have the concrete Completer implementations receive shared (e.g., Arc<RwLock<...>>) instances of the things they need. This would require the caller to instantiate them though, which was outside of the scope of this PR. We can perhaps return to this idea later, once we have a dynamic registry of completers.

@thegedge
Copy link
Contributor Author

@jonathandturner what do you think of this approach to decoupling?

I could also do a dyn CompletionContext. The context isn't really used on any hot path, so I wouldn't be worried about performance issues.

The major change in this commit is making the completion context be a trait.
This allows the completions crate to be completely decoupled from nu-cli,
allowing it to be developed independently from any frontend.
Copy link
Contributor

@sophiajt sophiajt 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.

As we work at this more, I think we should continue to separate rustyline from the completion engine. Maybe we only implement those traits if rustyline is detected (is that possible? not sure)

But yeah, this is good

@thegedge
Copy link
Contributor Author

As we work at this more, I think we should continue to separate rustyline from the completion engine. Maybe we only implement those traits if rustyline is detected (is that possible? not sure)

Agreed! There's actually only one use of rustyline in the completion engine right now, and that's for unescape. Honestly, I'd rather just drop that behaviour because it's weird that the completion engine can understand something that the rest of nushell cannot.

@sophiajt
Copy link
Contributor

@thegedge +1 for trait object. We can also make our own trait, so there's a trait between ours and rustyline's. Either way sounds good to me.

@gerred
Copy link

gerred commented Dec 20, 2020

Seems like #2241 is affected by this potentially, @jonathandturner should I base my code off of this branch?

@thegedge
Copy link
Contributor Author

This one is pretty old, @gerred . For the most part, it didn't introduce anything new, just moved the completion code into a separate crate. Since it's out of date, I think it would be best to start fresh instead of trying to rebase.

@thegedge thegedge closed this Dec 20, 2020
@thegedge thegedge deleted the remove-completion-dependency-on-context branch December 20, 2020 22:22
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