-
Notifications
You must be signed in to change notification settings - Fork 271
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
Add top-level commands to Dafny and redesign the CLI UI V2 #2603
Add top-level commands to Dafny and redesign the CLI UI V2 #2603
Conversation
@robin-aws Could we leave the The follow-up of merging this PR can be looking at |
@keyboardDrummer I can live with that. It does mean that we'll be stuck with carefully deprecating I split off #2807 to track that improvement separately. |
If there is a way to change to |
Adressing feedback on
I thought so too at first, but we ended up concluding the opposite and going with POSIX specifies that
What this means is that adding This is not the semantic that is being proposed here, since Another illustration of this might be: how do you run a Dafny file called This is the reason why GNU parallel uses Other tools use explicitly named arguments, like Btw, tools like If We could also decide to repurpose |
Would this be a scenario where it would be better to deviate from POSIX and adhere to what the npm/npx and dotnet are doing? The behavior of those tools seems to be what several people (me, @MikaelMayer, @seebees , @robin-aws ) were expecting. |
I think I'm missing something. As far as I can tell, both What I'm saying is: So using Or maybe I misunderstand the proposal and/or the way |
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 would suggest renaming EmitBinary, but otherwise great :)
@@ -12,7 +12,10 @@ public class OutputOption : StringOption { | |||
public override string ShortName => "o"; | |||
public override string ArgumentName => "file"; | |||
public override string Category => "Compilation options"; | |||
public override string Description => "filename and location for the generated target language files"; | |||
public override string Description => @" | |||
Specify the filename and location for the generated target language |
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 think the previous one was a bit better, since in other cases we use verbs for things that Dafny does (e.g. "print information")
@@ -11,7 +11,7 @@ public class PreludeOption : StringOption { | |||
public override string LongName => "prelude"; | |||
public override string ArgumentName => "file"; | |||
public override string Category => "Input configuration"; | |||
public override string Description => "choose Dafny prelude file"; | |||
public override string Description => "Choose the Dafny prelude file."; |
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.
Never mind the comment above, looks like we use verbs for non-verbs things as well.
Note that the C++ backend has various limitations (see | ||
Docs/Compilation/Cpp.md). This includes lack of support for | ||
BigIntegers (aka int), most higher order functions, and advanced | ||
features like traits or co-inductive types.".TrimStart(); |
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.
Is this rewrapped to 72 chars?
…into systemCommandline
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 ❤️ this so much. The streamlined help messages are a breath of fresh air, and I especially love the fact that we are better encapsulating Dafny's dependency on Boogie by not just allowing arbitrary passthrough options.
Based on playing around locally I couldn't find a way to display the old --help
text, which we do need for users that haven't migrated to the new UI yet. I'd suggest having a --legacy-help
option on the root command that prints that text, and perhaps adding a trailing line to draw attention to that ("Use --legacy-help to display help and usage information for using the CLI without commands")
Test/git-issues/model
Outdated
@@ -0,0 +1,859 @@ | |||
*** MODEL |
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.
Was this added accidentally or is it needed?
Source/DafnyCore/DafnyCore.csproj
Outdated
@@ -19,12 +19,14 @@ | |||
<DefineConstants>TRACE</DefineConstants> | |||
<TargetFramework>net6.0</TargetFramework> | |||
<PackageLicenseExpression>MIT</PackageLicenseExpression> | |||
<NoWarn>$(NoWarn);NU5104</NoWarn> |
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.
Better as a code comment than a PR comment :)
I've used CommandLineParser
in a couple of other projects (TestDafny and XUnitExtensions), would you mind just adding a comment somewhere in the code base on what we're using in System.CommandLine
that isn't in that library?
command.SetHandler(CommandHandler); | ||
} | ||
|
||
var rootCommand = new RootCommand("The Dafny CLI enables working with Dafny, a verification-aware programming language."); |
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.
When I try just dafny
on this branch I get:
$ dafny
*** Error: No input files were specified in command-line .
I think we can safely break that behavior so we get a better default help message.
public class ShowSnippetsOption : BooleanOption { | ||
public static readonly ShowSnippetsOption Instance = new(); | ||
|
||
public override string LongName => "showSnippets"; |
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.
AFAICT we standardized on hyphen-separated names (--no-verify
, --verification-time-limit
, etc), is there a reason this isn't --show-snippets
?
|
||
namespace XUnitExtensions.Lit { | ||
|
||
// TODO: Make safely immutable | ||
public class LitTestConfiguration { | ||
public readonly Dictionary<string, string> Substitutions; | ||
public readonly Dictionary<string, object> Substitutions; |
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.
This looks like a useful change, but can you document what actual types are supported as substitution values? AFAICT it's either string
or IEnumerable<string>
.
I stand corrected, I didn't realize |
Co-authored-by: Clément Pit-Claudel <cpitclaudel@users.noreply.github.com>
Fixes #2036
Fixes #2360
Changes
verify
,integrate
,run
, as defined in Introduce top level commands commands for Dafny #2036-
and--
for short and long names, instead of-
and/
for long names)=
instead of:
for specifying the value of an option in a single term--<option>
will mean the same as--<option>:true
for boolean options--boogie="/loopUnroll:3 /overlookTypeErrors"
vscCores
is exposed ascores
useBaseNameForFileName
is exposedtimeLimit
is exposed asverificationTimeLimit
compileTarget
is renamed totarget
dprint
is renamed toprint
print
is renamed tobprint
compileVerbose
has been inverted and renamed toquiet
print
(previously named dprint),rprint
,bprint
,printMode
,useBaseNameForFileName
showSnippets
is an existing option that has been refactored to make use of this new mechanism.compile
,spillTargetCode
anddafnyVerify
when using commands.dafny <command> --help
shows which options there are for that specific command.Output from
dafny --help
:Output from
dafny verify --help
:Output from
dafny run --help
Future work
--boogie="/print=<file"
asdafny build --target=boogie
--boogie
--noVerify
so it does not translate to and invoke Boogie.Testing
comp/separateCompilation/usesTimesTwo.dfy
andgit-issues/github-issue-305.dfy
usedafny build
comp/Arrays.dfy
andcomp/AsIs-Compile
usedafny verify
anddafny run
comp/AsIs-Compile
also uses shortname optionsdafny0/AssumptionVariables1
uses boogie options through--boogie-<optionName>
git-issue-2197.dfy
tests the boolean optionshowSnippets
without explicit truthy value, whileShowSnippets.dfy
tests it with explicit thruthy value.By submitting this pull request, I confirm that my contribution is made under the terms of the MIT license.