Refactor #2

Michael-Klassen · 2015-10-09T18:01:55Z

Refactor DateTimeParser

- Generating Dict similar to those used in Base.Dates eg. DAYOFWEEKTOVALUE

codecov-io · 2015-10-09T18:02:09Z

Current coverage is `100.00%`

Merging #2 into master will not affect coverage as of ac867e8

@@            master      #2   diff @@
======================================
  Files            1       1       
  Stmts          314     317     +3
  Branches         0       0       
  Methods          0       0       
======================================
+ Hit            314     317     +3
  Partial          0       0       
  Missed           0       0

Review entire Coverage Diff as of ac867e8

Powered by Codecov. Updated on successful CI builds.

Michael-Klassen · 2015-10-09T18:09:06Z

Would like to "Perform lowercase earlier in _parsedate. Probably just do map(lowercase, X).
Then you can remove the random lowercase calls peppered throughout that function" but timezone_infos is case sensitive.

Michael-Klassen · 2015-10-09T18:12:01Z

"Eventually res could become res = Union{Period,TimeZone}[]. When you are ready to make a Date you can ZoneDateTime(res...). That will call ZonedDateTime(parts::Union{Period,TimeZone}...). Both Date and DateTime also have these constructors"

Michael-Klassen · 2015-10-09T18:12:13Z

"I'd also like _parsedate to be broken into smaller functions. For example extract just the timezone from the tokens"

omus · 2015-10-13T18:56:30Z

src/DateTimeParser.jl

+    return i
+end
+
+function processymd!(res::Dict, ymd::Array, mstridx=-1; yearfirst=false, dayfirst=false)


Why not just have processymd return a tuple of Year, Month, Day? ymd should be ymd::Array{Int}. mstridx is a bad variable name and should be changed.

Ok, I'll change it to return a tuple instead of passing res by reference. Any preferences on what I should rename mstridx to?

month_index?

The reason why processymd does not just return a Year, Month and Day is because there isn't always a Year, Month and Day to return.

omus · 2015-10-13T20:00:23Z

src/DateTimeParser.jl

                    i += 2
                    if i+1 <= len && tokens[i] == "." && isdigit(tokens[i+1])
-                        res["millisecond"] = round(Int, 1000 * parse(Float64, string(".", tokens[i+1])))
+                        temp = 1000 * parse(Float64, string(".", tokens[i+1]))
+                        res["millisecond"] = Millisecond(round(Int, temp))
                        i += 2
                    end
                end


Maybe this section is a little too fragile?

julia> parse(DateTime, "5:4.5") 2015-01-01T05:04:30 julia> parse(DateTime, "5:4:3.5") 2015-01-01T05:04:03.5 julia> parse(DateTime, "5:4.5:3.5") ERROR: Failed to parse date in _parsedate at /Users/omus/.julia/v0.4/DateTimeParser/src/DateTimeParser.jl:361 in parse at /Users/omus/.julia/v0.4/DateTimeParser/src/DateTimeParser.jl:92

Ending with a decimal is a special case. I'm not sure we want to support having a floats in the middle.

That's fair

omus · 2015-10-13T21:03:07Z

Good work overall!

omus · 2015-10-15T19:26:23Z

Rename package to "DateParser.jl"? There won't be a perfect name as this covers Date, DateTime, and ZonedDateTime types but I think DateParser is the best
Add appveyor badge to README
JUMP, PERTAIN, UTCZONE should be Arrays rather than Tuples so they can be extended

omus · 2015-10-15T20:20:44Z

src/DateTimeParser.jl

+    res.month = get(res.month, Month(default))
+    res.day = get(res.day, Day(default))
+
+    return Date(get(res.year), get(res.month), get(res.day))


Could shorten the code to:

return Date( get(res.year, year(default)), get(res.month, month(default)), get(res.day, day(default)), )

omus · 2015-10-15T20:33:12Z

src/DateTimeParser.jl

@@ -152,25 +167,26 @@ function _parsedate(datetimestring::AbstractString; fuzzy::Bool=false,
            # Token is a number
            i += 1 # We want to look at what comes after the number
            if length(ymd) == 3 && tokenlength in (2,4) &&
-                (i>=len || (tokens[i] != ":" && !haskey(HMS, lowercase(tokens[i]))))
+                (i>=len || (tokens[i] != ":" && !haskey(HMS[locale], lowercase(tokens[i]))))


Condition can be changed to length(ymd) == 3 && tokenlength in (2,4) && i > len without causing the testcases to fail. Add additional tests?

Ok, I'll add a test for it parse(DateTime, "19991212 0259+1:00")

Michael-Klassen · 2015-10-16T15:56:17Z

The 32 bit win build is broken because the released TimeZones 32 bit build is broken

omus · 2015-10-16T16:09:14Z

Which is broken because LightXML 32-but is broken because WinRPM 32-bit is broken. All good

Refactor

omus and others added 2 commits October 9, 2015 10:11

Standardized translations for date names to values INCOMPLETE

9f1c0bf

- Generating Dict similar to those used in Base.Dates eg. DAYOFWEEKTOVALUE

Add processymd function

88fa83e

Michael-Klassen added 9 commits October 9, 2015 13:51

Clean up parse functions args

2cb98c9

Move code

73c3c70

Use Period types (Year, Hour, Minute, etc...)

5ffe744

Finish incomplete changes

70109dd

Pull standardize branch

529f24b

Make DayOfWeek a subtype of Period

785b0a8

Call Period... constructors

f391c71

Refactor parsing timezone

89f62cd

Remove unused value

61f4506

omus reviewed Oct 13, 2015
View reviewed changes

Rename mstridx to monthindex

938f6cf

omus reviewed Oct 13, 2015
View reviewed changes

Michael-Klassen added 2 commits October 14, 2015 12:37

Don't assume 6 digits is YYMMD

d1f216b

Make res a custom type

14e787d

Michael-Klassen added 8 commits October 14, 2015 13:36

Abstract repeated code as function

5d7d21b

"4.5 hours 6 minutes" should be 4:36

9e360e3

Change convertyear and converthour to use Ints again

6bd3eae

Refine timezone support

4c96447

Remove unused line

199648e

Fix locale

3162ed6

Add locale tests

cba7d8f

Check if tzoffset's hours or minutes are out of range

0d4fcdb

Michael-Klassen added 3 commits October 15, 2015 14:42

Add local support for HMS and AMPM

aa7aa04

Add tests for weird whitespace and tzname interaction

00e88f6

Change JUMP, PERTAIN, UTCZONE to be Arrays so they can be extended

26c0d73

omus reviewed Oct 15, 2015
View reviewed changes

Rename package to DateParser.jl

0702519

omus reviewed Oct 15, 2015
View reviewed changes

Michael-Klassen added 5 commits October 15, 2015 15:39

Add appveyor build status badge

5682824

Add test

7dda934

Shorten the code

dad543c

Rename variables

0276a50

Clean up code

a2d83de

Michael-Klassen added 3 commits October 16, 2015 11:30

Try to stay within a 92 char line length

ea93548

Rearrange tests

9060155

Add more tests

fe84f67

Michael-Klassen added a commit that referenced this pull request Oct 16, 2015

Merge pull request #2 from invenia/refactor

9537810

Refactor

Michael-Klassen merged commit 9537810 into master Oct 16, 2015

Michael-Klassen deleted the refactor branch December 1, 2015 15:54

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor #2

Refactor #2

Michael-Klassen commented Oct 9, 2015

codecov-io commented Oct 9, 2015

Michael-Klassen commented Oct 9, 2015

Michael-Klassen commented Oct 9, 2015

Michael-Klassen commented Oct 9, 2015

omus Oct 13, 2015

Michael-Klassen Oct 13, 2015

omus Oct 13, 2015

Michael-Klassen Oct 13, 2015

omus Oct 13, 2015

Michael-Klassen Oct 13, 2015

omus Oct 13, 2015

omus commented Oct 13, 2015

omus commented Oct 15, 2015

omus Oct 15, 2015

omus Oct 15, 2015

Michael-Klassen Oct 15, 2015

Michael-Klassen commented Oct 16, 2015

omus commented Oct 16, 2015

Refactor #2

Refactor #2

Conversation

Michael-Klassen commented Oct 9, 2015

codecov-io commented Oct 9, 2015

Current coverage is 100.00%

Michael-Klassen commented Oct 9, 2015

Michael-Klassen commented Oct 9, 2015

Michael-Klassen commented Oct 9, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

omus commented Oct 13, 2015

omus commented Oct 15, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Michael-Klassen commented Oct 16, 2015

omus commented Oct 16, 2015

Current coverage is `100.00%`