Skip to content

Add support for TimeSpan#205

Merged
nemec merged 3 commits into
commandlineparser:developfrom
0xced:TimeSpan
Jan 8, 2018
Merged

Add support for TimeSpan#205
nemec merged 3 commits into
commandlineparser:developfrom
0xced:TimeSpan

Conversation

@0xced
Copy link
Copy Markdown
Contributor

@0xced 0xced commented Jan 5, 2018

This is a work in progress, please do not merge yet.

@0xced
Copy link
Copy Markdown
Contributor Author

0xced commented Jan 5, 2018

It looks fine now. You can merge if you think this is a good addition.

@0xced
Copy link
Copy Markdown
Contributor Author

0xced commented Jan 5, 2018

Oh and by the way, I found why 30 tests were failing: because Console.WindowWidth returns 0 when running under mono leading to ArgumentOutOfRangeException being thrown because maximumLength < 1.

0xced added 2 commits January 8, 2018 10:41
The purpose of this commit is to trigger CI on AppVeyor to verify that all tests are passing since 30 of them do not pass on my machine (macOS 10.12.6 with JetBrains Rider 2017.3 and mono 5.4.1.7)
@nemec nemec changed the base branch from master to develop January 8, 2018 20:32
@nemec nemec merged commit 93cdf61 into commandlineparser:develop Jan 8, 2018
@0xced 0xced deleted the TimeSpan branch January 8, 2018 20:35
@nemec
Copy link
Copy Markdown
Contributor

nemec commented Jan 8, 2018

This is great - not only will it work for TimeSpans, but any class with a TypeConverter should be supported by the library now.

Thank you for contributing, @0xced!

@ericnewton76
Copy link
Copy Markdown
Member

@0xced hey cedric, regarding the tests and the maximumLength, you are exactly right. I was trying to find some time to implement that aspect in the testing apparatus to force that variable to always be 80 instead of -1 if the Console.WindowWidth access failed. You'll probably see a pull request from me regarding that at some point

@0xced
Copy link
Copy Markdown
Contributor Author

0xced commented Aug 6, 2018

I noticed that this pull request was merged (into develop) but only commit 6e52805 was cherry-picked for version 2.2.1. Are you going to merge it in into master for the next release?

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants