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

empty OptionList doesn't show error #68

Closed
turch opened this issue Mar 13, 2013 · 10 comments
Closed

empty OptionList doesn't show error #68

turch opened this issue Mar 13, 2013 · 10 comments

Comments

@turch
Copy link

turch commented Mar 13, 2013

I just started using this library, so forgive me if this is stupid request:

When an empty option list is parsed, shouldn't a format error be added to the PostParsingState? As I'm sure you know, It would be a one line change to add

DefineOptionThatViolatesFormat(option);

to the block that tests whether the option is empty. (OptionGroupParser.cs L54)

e.g.

class Options
{
   [OptionList('d', "date-range", Separator = ':')]
   public List<string> DateRange { get; set; }
}
>foo.exe -d

   -d/--date-range option violates format

>
@gsscoder
Copy link
Owner

Hi, thanks for reporting.

You're working on development or stable branch? For the name of the method I argue the first...

@turch
Copy link
Author

turch commented Mar 14, 2013

I'm working on the master branch. I've sent you a PR so you can see what I did to get the desired behavior (but I see now that ArgumentParser.cs is gutted in the dev branch so it's probably irrelevant).

turch added a commit to turch/commandline that referenced this issue Mar 14, 2013
@gsscoder
Copy link
Owner

@turch, no it's not irrelevant... Often between me and contrib we use PR to show up problems.

Could be my fault, in the issues there's an announcement

#67

called "Coordinate PR to development branch" that I should rename to "Coordinate PR".

In this time main kernel is under heavy refactoring and I'm not able to accept PR...

I'll clarify it in the informative issue.

Anyway thank for using the library and for your interest in the project! :)

Please accept this state of art for the moment and please take as good this reply for other issue.

Thanks again,
Giacomo Stelluti Scala

P.S.: all work done is not lost and will evaluated and eventually integrated in 2.0, along with credits.

@gsscoder
Copy link
Owner

New 2.0 pre-rel automatically handle collection targets using always [Option] attribute. I'm going to check if there's unit test coverage for this, if not I'll add one.

@gsscoder
Copy link
Owner

Sorry, please not take into account last post...

This feature lacks in 2.0 pre-rel, it will be implemented ASAP.

So I mark the issue as enhancement in respect of master branch.

@gsscoder
Copy link
Owner

I'm experimenting on issue68 branch (derived from master and still work in progress) how to implement previous OptionList adding only a Separator property to the model (both Option and Value attrs).

When targeting IEnumerable<T> two things could happens:

  • the single argument is expanded after being divided by Separator and everything proceeds as with normal sequence parsing
  • or we can treat it as particular conversion in which the value is processed by string.Split(...)

cc/ @turch @nemec

Some reasoning is necessary... I want to keep the core of 2.0 as compact as possible.

@nemec
Copy link
Collaborator

nemec commented Jun 10, 2015

In the master branch, it seems like there is no way to explicitly say you want to parse multiple parameters, right? That knowledge is gained from a combination of OptionAttribute + IEnumerable<T> type.

So let's say, without any additional configuration would you allow this as a valid input?

[Option('i')]
public IEnumerable<int> Integers { get; set; }

$ example.exe -i 1 2 3 4

If so, it may be more "clean" to do your first suggestion of splitting -i 1:2:3:4 into a sequence of arguments -i 1 2 3 4 and continuing with normal parsing. However, adding a separator might create duplicate functionality (with the separator vs. with spaces) and allow some weird situations:

$ example.exe -i 1:2:3 4 5 6:7 8

I'd be inclined to say that whitespace is always the separator and if you want something custom you could apply a custom TypeConverter to the property (which will allow the user to perform any custom conversion)

@gsscoder
Copy link
Owner

@nemec, yes you'll right. As now is impossible in 2.0...! I've started something (still not completed -> in a branche named issue68.

  • DONE: I've added a Separator property to public attributes and specification model types.
  • TODO: "cheat" the parser when Separator has a value != "" (or it can be redefined Maybe<string> just in the model), split the arg and explode it in the token sequence.

Thanks for comments.

@gsscoder
Copy link
Owner

I'm about to merge issue68 -> master... I've temporary added Separator to both Option and Value, but I think that only Option should have it (so it's consistent with latest stable).

I'll revert back this thing for Value attr, add few test and merge to master.

cc/ @nemec @mizipzor @gimmemoore

gsscoder added a commit that referenced this issue Jun 12, 2015
gsscoder added a commit that referenced this issue Jun 12, 2015
@gsscoder
Copy link
Owner

[Option('i', Separator=":")]
public IEnumerable<int> Integers { get; set; }

This is now possible... Is the old OptionList. I close the issue. I'm wondering if it's better define the Separator as System.Char.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants