-
Notifications
You must be signed in to change notification settings - Fork 36
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
date handling: failing tests, chronic causes different behavior #76
Comments
This is tricky. So it will be tricky to run tests that fail if the gem is not installed There is the |
@kbrock ic, perhaps an explicit message that tests are failing due to a missing gem would suffice. Trollop 1.2.1 was masked in Gentoo with recent Ruby versions simply because maintainer didn’t investigate deep enough. |
#parse_data_parameter tries to load chronic and if it fail to load, then the Date behavior of Trollop is "normal". Maybe the tests either need to have chronic as a hard dependency, or bypass the tests that would fail if chronic was not available. Again, i'm not really a big fan of the behavior changing based on existence of another installed gem - in a group environment (e.g. a University), someone may install a gem (chronic) into a "global" Ruby install and then all of the sudden command line tools that use Trollop would start behaving oddly. That would be pretty hard to track down. 😕 |
I got test failures when i ran, caused by issues in the Date handling.
The tests are providing U.S. formatted dates (e.g. 5/1/2010) but Date.parse expects Day/Month/Year format like is used in the E.U.
bypassing that test, the 'today' in test_date_formatting also caused breakage.
Both of these were fixed by installing the chronic gem.
I don't know the history behind this, so maybe this is intentional, but IMHO, an optional dependency shouldn't change existing functionality. Perhaps another way to handle could be a way to register a field (e.g. "%m/%d/%Y", "%Y/%m/%d", etc) for use with Time.strptime, though that would break the relative time feature that Chronic adds.
The text was updated successfully, but these errors were encountered: