-
Notifications
You must be signed in to change notification settings - Fork 23
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
refactor repl commands, add :help command #230
Conversation
longHelp = "", | ||
category = Dev, | ||
cmdtype = ShellCmd, | ||
action = handleAnn, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I no longer see any benefit to separating out the definition of the actions like this. It just means the code for each command is spread out over multiple parts of the file. At the very least I think we should move the definition of each action to right after the definition of its REPLCommand
, so all the code for each command will be in one place.
src/Disco/Interactive/Commands.hs
Outdated
category = Dev, | ||
cmdtype = ShellCmd, | ||
action = handleTypeCheck, | ||
parser = TypeCheck <$> parseTypeTarget |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we also move the definition of parseTypeTarget
to right here? Unless I am mistaken this is the only place it is used.
src/Disco/Interactive/Commands.hs
Outdated
iputStrLn "" | ||
where | ||
sortedList cmds = sortBy (\(SomeCmd x) (SomeCmd y) -> compare (name x) (name y)) $ filteredCommands cmds | ||
-- remove "builtins" (let, eval, etc), don't show dev-only commands by default |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be nice if we could find a good way to show built-in commands too, though I think that can probably be put off to the future.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@byorgey I added build-in commands to the help output.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great!
2f11cfd
to
b3329b7
Compare
11de87a
to
b3329b7
Compare
src/Disco/Interactive/Commands.hs
Outdated
usingCmd = | ||
REPLCommand { | ||
name = "using", | ||
helpcmd = ":using <extension>", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't have a colon in front of :using
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed, is using
a dev-only command?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, using
is for end users to turn on language extensions.
@@ -91,342 +91,321 @@ annCmd :: REPLCommand 'CAnn | |||
annCmd = | |||
REPLCommand { | |||
name = "ann", | |||
helpcmd = ":ann", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like the idea of having a helpcmd
field. However, let's not put explicit tab characters in them --- both because we should avoid using tabs in the output (different terminals etc. may display them differently) and also because it's fragile. Instead let's make the code to display the help more general, so it figures out the maximum length of all the helpcmd
fields it's being asked to display, and pads things out with spaces approriately.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
agreed on tabs, I'll push another change to this PR soonish.
8e13e93
to
b3329b7
Compare
b3329b7
to
f47f9c9
Compare
@byorgey removed tabs, padding |
GADT/typed indices implementation from byorgey via #229
541913c
to
21841a3
Compare
@@ -0,0 +1,2 @@ | |||
:help | |||
1+1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Restyled.io wants to strip the whitespace after the :help
command, so I added a simple 1+1
to produce additional output.
Looks good to me! |
GADT/typed indices implementation from @byorgey via #229
see also: