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

Address #27 from fsprojects/Projekt #32

Open
wants to merge 8 commits into
base: master
Choose a base branch
from
Open

Address #27 from fsprojects/Projekt #32

wants to merge 8 commits into from

Conversation

baronfel
Copy link

I took a pass at unifying the command line args according to issue #27 . I think CommandLine.Parser will work better for this case, due to the command -> verb -> options pattern this utility has.

I added a basic test that verifies that the new implementation is equivalent to the old implementation, but what I'd really like to do is write an FsCheck test that uses the Operation type to generate a command line string via the old parser, and then parse that string using the new parser and compare the resulting Operations.

@rneatherway
Copy link
Contributor

@baronfel: thanks a lot for taking a look at this. How do you think your approach affects the ease of automatically generating the docs from the usage text like Paket does? At the moment I copied and pasted manually into https://github.com/fsprojects/Projekt/blob/master/docs/content/index.md, but I'd like to avoid that in the future.

@baronfel
Copy link
Author

baronfel commented Aug 2, 2015

This library will also do the automatic command line usage generation. The 'description' attribute parameters on the verbs and options are what is used automatically. I can post a screenshot here later after a few pints ;) I think I like this library a bit more than the current rev of UnionArgsParser for verbs.

@baronfel
Copy link
Author

baronfel commented Aug 3, 2015

image

So this is what it looks like out of the box. But using the included API I can make this look however you like. Any pointers/requests? I'm guessing to keep it like the previous version?

@kjnilsson
Copy link
Collaborator

Looks ok to me however I'd like the code formatting to follow common formatting guidelines a bit closer: https://github.com/dungpa/fantomas/blob/master/docs/FormattingConventions.md, especially around records.

Some lines are also a bit too long. Try keeping them below 120 if possible.

…Also could purge the version Operation because the new args lib handles that.
@baronfel
Copy link
Author

baronfel commented Aug 3, 2015

Is this more in line? I was able to condense some of the long lines as well as correct the newline/braces situation with the records. I also forgot to annotate the positional parameters with MetaNames, so now the positional parameters have help entries.

| None -> Library
| _ -> failwith "invalid template argument specified"
[<Verb("init", HelpText = "create a new project")>]
type public InitOptions =
Copy link
Collaborator

Choose a reason for hiding this comment

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

no need for the 'public' accessor

Copy link
Author

Choose a reason for hiding this comment

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

thanks, will fix.

@baronfel
Copy link
Author

baronfel commented Aug 3, 2015

Ok, this commit should address the pieces that you brought up. Thanks for the detail there.

@@ -1 +1,2 @@
UnionArgParser
Copy link
Contributor

Choose a reason for hiding this comment

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

Can this be removed?

Copy link
Author

Choose a reason for hiding this comment

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

yes, I missed it when I purged it from the top-level paket.references.

SourceFiles="@(Templates)"
DestinationFolder="$(OutputPath)\templates\%(Templates.RecursiveDir)"
/>
<Copy SourceFiles="@(Templates)" DestinationFolder="$(OutputPath)\templates\%(Templates.RecursiveDir)" />
Copy link
Contributor

Choose a reason for hiding this comment

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

The Templates ItemGroup has been removed so this doesn't work anymore.

Copy link
Author

Choose a reason for hiding this comment

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

Crap, this happened because the templates node caused me to be unable to build in VS2015. Do you all have this same problem?

image

(it may be a 2015 problem)

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, I had that problem with VS as well, but I don't normally use VS! I just tried splitting it into a separate file, so create a new Template.targets:

<Project xmlns="http://schemas.microsoft.com/developer/msbuild/2003">
<ItemGroup>
  <Templates Include="$(SolutionDir)\templates\**\*.*" />
</ItemGroup>
</Project>

and use <Import Project="Template.targets" />. Does that trick VS?

Copy link
Author

Choose a reason for hiding this comment

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

Yeah, that worked. So this is one of those times where I begin to realize how much I don't know. Does msbuild not evaluate the Imported elements with the same level of rigor as top-level nodes?

Copy link
Contributor

Choose a reason for hiding this comment

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

No it absolutely does, but msbuild understands the wildcard perfectly well, I was hoping that the over-eager VS check was implemented separately.

@rneatherway
Copy link
Contributor

I really like the look of this! Thanks a lot for the PR. Could you tidy the help output up a bit more though please?

  1. No blank line between options.

  2. Don't output --help and --version in the help for every verb.

  3. Preserve the help text for the options from the old version. For example for direction under movefile, we should say that the choices are (up|down) as before. Same goes for most of the options, just to make it clear what the acceptable range is.

    $ bin/Projekt/Projekt.exe help movefile
    Projekt 0.0.2.0
    Copyright (C) 2015 fsprojects
    
    --direction              Required. 
    
    --repeat                 (Default: 1) 
    
    --help                   Display this help screen.
    
    --version                Display version information.
    
    project path (pos. 0)    Required. 
    
    file path (pos. 1)       Required. 
    
    
  4. Rather than listing the positional arguments like above, is it possible to use the more standard approach of a header of the form: Project.exe movefile <project path> <file path>?

| ToLower "up" -> Up
| ToLower "down" -> Down
| _ -> failwith "invalid direction specified"
[<Verb("reference", HelpText = "reference a dll")>]
Copy link
Contributor

Choose a reason for hiding this comment

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

Currently this verb is for referencing projects. Referencing dlls is planned: #23 (@kjnilsson you fancy it?)

Copy link
Author

Choose a reason for hiding this comment

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

I'll clean that up for sure.

@baronfel
Copy link
Author

baronfel commented Aug 3, 2015

@rneatherway thanks for the feedback on the help text. What we've got there is completely out of the box, and I can customize it to a great extent. I'll spend some time making the text in line with your points.

@baronfel
Copy link
Author

baronfel commented Aug 5, 2015

There's a bit of a roadblock with this PR, because the version of CommandLine.Parser that supports verbs doesn't allow for the same level of customization as previous versions.

It used to be that you could hijack and overwrite the entire help generation, but now you can only use attributes and examples to guide the automatic generation of help usage, as briefly described in (this issue)[https://github.com/gsscoder/commandline/issues/116].

So I can't really address your concerns right now, @rneatherway. :( Maybe we put this one on the back burned for a bit until I or original dev can extend help generation.

@rneatherway
Copy link
Contributor

@baronfel that's a real shame, thanks a lot for taking the time to have a crack at this in any case. Hopefully it will be implemented soon, the project does seem to be actively developed right now and it's a fairly important thing.

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.

3 participants