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

execute multiple instructions #8

Merged
merged 1 commit into from
Aug 13, 2017
Merged

Conversation

KodeAndGame
Copy link
Contributor

can now enter multiple instructions, separated by semi-colon, into one command entry

can now enter multiple instructions, separated by semi-colon, into one command entry
@KodeAndGame
Copy link
Contributor Author

hey not sure if you accept PRs but thought I'd give it a shot!

@discosultan
Copy link
Owner

Hey @KodeAndGame, thanks for the contribution! I like the idea of being able to run multiple commands per line. And pull requests are always welcome :)

I'm thinking out loud here:

  • Currently you will lose autocompletion capability after first command. It'd be nice if the AutoComplete function in ManualInterpreter.cs was also aware of the the instruction separator. I don't remember the code too well, but maybe it'd be enough if this line was changed so that it would take the instruction substring of input.Value based on current input.CaretIndex. TryAutocomplete will probably also require modifications.

  • It'd be nice all the separators as well as case sensitivity were exposed as public properties on ManualInterpreter. This would allow more flexibility for the consumers.

@KodeAndGame Just to clarify, I don't expect you to make the aforementioned changes. They are just ideas for future reference.

@discosultan discosultan merged commit 19d2b19 into discosultan:master Aug 13, 2017
@KodeAndGame
Copy link
Contributor Author

That's great feedback. I know you're just thinking out loud but seeing as the experience is important to the what I'm building now, I'd be interested into fixing your first point. I'll start looking into it.

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