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

Pr/180 #181

Merged
merged 3 commits into from May 18, 2015
Merged

Pr/180 #181

merged 3 commits into from May 18, 2015

Conversation

milktrader
Copy link
Contributor

I pulled #180, corrected the path for the new tests

@coveralls
Copy link

Coverage Status

Coverage increased (+0.87%) to 76.55% when pulling 882282e on pr/180 into d39db57 on master.

@milktrader
Copy link
Contributor Author

Fixing the problem behind this PR is something myself and @quinnj have put on the back burner. The kwarg where a format is specified should cover edge cases, but there needs to be a better parsing of possible combinations out there. I'm not really fond of how R has this hurdle to their odd time series conversions.

Here's the plan.

Track down where in Dates.jl datetime is parsed and simply update the regex engine being used there.

@multidis
Copy link
Contributor

Would be helpful to merge the kwarg option so that it becomes usable in the master package version. Oftentimes I have to do multiple steps to reformat timestamps the way TimeSeries.jl can take it and passing the format string would solve that.

Even if a more robust regex parsing is introduced in the future (which may also depend on Julia 0.4 Base-dates support modifications), it is always helpful to have an option for the user to specify the exact format instead of hoping for correct parsing.

@milktrader
Copy link
Contributor Author

Gotcha, good point. Not waiting for the perfect solution at the expense of a useful one. Let me get the documentation up to speed and we'll get this merged and tagged

@milktrader
Copy link
Contributor Author

@multidis I changed the dtformat to simply format with the bold assumption that users will understand that format is a date format. It's also documented.

@milktrader
Copy link
Contributor Author

fwiw, the zoo package in R uses the simple format kwarg.

@milktrader
Copy link
Contributor Author

gah, got this on nightlies

ERROR: LoadError: LoadError: LoadError: LoadError: LoadError: UndefVarError: Range1 not defined

@coveralls
Copy link

Coverage Status

Coverage increased (+0.43%) to 76.11% when pulling 401ecf1 on pr/180 into d39db57 on master.

@milktrader
Copy link
Contributor Author

I'm going to merge this but hold off on tagging it until I can figure out what happened to Range1. I tried a quick Compat workaround but it didn't work.

milktrader added a commit that referenced this pull request May 18, 2015
@milktrader milktrader merged commit bdcb7fa into master May 18, 2015
@milktrader
Copy link
Contributor Author

This has been included in tag 0.5.8 now. Needed to navigate a couple extra ERRORs.

@milktrader milktrader deleted the pr/180 branch May 18, 2015 21:19
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.

None yet

3 participants