Skip to content
This repository has been archived by the owner on Oct 5, 2023. It is now read-only.

Denote commands with a / #137

Merged
merged 3 commits into from Dec 13, 2019
Merged

Conversation

dyc3
Copy link
Contributor

@dyc3 dyc3 commented Dec 12, 2019

This makes it so that commands must start with a /, and splits the input by spaces to get arguments for commands.

My reasoning for choosing / specifically is because users (especially gamers) will be more familiar with using / to denote commands (eg minecraft)

@nickwalton
Copy link
Contributor

nickwalton commented Dec 13, 2019

What happens if the input is the empty string? I believe you'll get an error.

@Renha
Copy link
Contributor

Renha commented Dec 13, 2019

It's not very convenient to type in / using say mobile phone

@spenserblack
Copy link
Contributor

spenserblack commented Dec 13, 2019

@Renha 🤔 This value should probably be something that nobody would ever start a sentence with, either purposefully or accidentally. I could see . being more convenient as a command escape (the Node.js REPL uses this, so I guess it's not too uncommon) for both PC and mobile phone users, so maybe that should be considered.

But personally I agree with @dyc3 that it should be /. I have terrible typing accuracy on mobile, and an "inconvenient" character would definitely prevent me from making mistakes 😆

else:
console_print("Saving has been turned off. Cannot save.")
else:
console_print(f"Invalid argument: {args[0]}")
Copy link
Contributor

Choose a reason for hiding this comment

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

Is the minimum Python version required for AIDungeon documented anywhere? f-strings require Python 3.6, which shouldn't conflict with the Colab runtime, but may conflict with users running AIDungeon locally.

I believe tensorflow 1.15 requires Python 3.5 or greater, so this might bump the required Python version up a minor version.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

IIRC, the install script specifically checks for python 3.6

Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry, completely missed that. Thanks!

@dyc3
Copy link
Contributor Author

dyc3 commented Dec 13, 2019

@nickwalton Good catch, will fix.

@Renha We have mobile players? Even then, I'm pretty sure during a normal session commands wouldn't be used very often, so convenience shouldn't be a huge factor.

@dyc3 dyc3 mentioned this pull request Dec 13, 2019
@ben-bay
Copy link
Contributor

ben-bay commented Dec 13, 2019

I'm good with this, after you add it to CHANGELOG.md :)

@dyc3
Copy link
Contributor Author

dyc3 commented Dec 13, 2019

@ben-bay done!

@ben-bay ben-bay merged commit 5d69b00 into latitudegames:develop Dec 13, 2019
@dyc3 dyc3 deleted the denote-commands branch December 13, 2019 22:56
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants