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

Parametrize Editor by the Completer type #52

Merged
merged 1 commit into from
Jul 18, 2016

Conversation

Marwes
Copy link
Contributor

@Marwes Marwes commented Jul 15, 2016

This allows the Editor to implement Sync as long as it contains a completer which implement Sync. It lets the Editor take ownership of the completer.

Fixes #49

@Marwes
Copy link
Contributor Author

Marwes commented Jul 15, 2016

Happy to update this PR if you find any problems with it. It may be a bit awkward that users need to explicitly parametrize Editor with the completer if it is unused but I am not sure what is the best way to avoid that.

@kkawakam
Copy link
Owner

kkawakam commented Jul 16, 2016

Hey @Marwes, sorry that I didn't get back to you on your issue during the week. I agree that with you that it is a bit awkward with explicitly parametrizing Editor if there is no completer but I am okay with it. Can you update the README.md example to reflect that change?

I also just merged in another branch into master and it has caused some conflicts with your branch.

@Marwes Marwes force-pushed the parameterized_editor branch from 043cc38 to 83664d9 Compare July 16, 2016 09:16
This allows the `Editor` to implement `Sync` as long as it contains a completer which implement `Sync`. It lets the `Editor` take ownership of the completer.
@Marwes Marwes force-pushed the parameterized_editor branch from 83664d9 to 8c98597 Compare July 16, 2016 09:18
@Marwes
Copy link
Contributor Author

Marwes commented Jul 16, 2016

No problem I am in no rush, just wanted to make sure making the change was possible.

A tip for keeping the README example updated, skeptic is really useful to ensure that the examples in the README always builds (though it can be a bit slow if you start having 10+ examples as I have found).

@kkawakam kkawakam merged commit 480bf15 into kkawakam:master Jul 18, 2016
@Marwes Marwes deleted the parameterized_editor branch July 18, 2016 08:31
@Marwes
Copy link
Contributor Author

Marwes commented Jul 18, 2016

👍

@kkawakam
Copy link
Owner

kkawakam commented Jul 29, 2016

@kkawakam note to self: indicate in release notes this is a breaking change because of the need to explicitly parametrize the Editor when there is no completer.

@Marwes
Copy link
Contributor Author

Marwes commented Aug 1, 2016

@kkawakam I used the pattern of () as a 'null object' in this PR which may not be the most intuitive solution. I used this pattern in gluon as well but I have been convinced to change it to an explicit type for the null object instead to increase discoverability (so maybe a pub struct NullCompleter; instead).

I obviously don't mind () myself but I thought I would let you know that it might not be a common pattern that is easily understandable.

gwenn pushed a commit to gwenn/rustyline that referenced this pull request Nov 30, 2018
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.

2 participants