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

util/types: provide time Format method #2206

Merged
merged 10 commits into from
Dec 12, 2016
Merged

Conversation

tiancaiamao
Copy link
Contributor

use Format to implement String(), so 00 month and day can be display
copy code mostly from evaluator builtinDateFormat, we can clean up them, later.

parseFrac overflow is irrelevant, and already in another PR, so it can be ignored when review.

package types

import (
"fmt"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

add a blank here

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

str, t.Expect))
}

// error
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

add a FIXME here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

there is already a TODO in the following line

return ErrInvalidTimeFormat.Error()
}
return t1.Format(DateFormat)
// we'll control the format, so no error would occur.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we->We

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If err is not nil, print a error log?

// Get the final frac, with 6 digit number
// 0.1236 round 3 -> 124 -> 123000
// 0.0312 round 2 -> 3 -> 30000
return int(round * math.Pow10(MaxFsp-fsp)), nil
// 0.9999999 round 6 -> 1000000 -> overflow
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

move this comment above line 88?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this would not be included in this PR, it not diff master, but diff
tiancaiamao/timestamp-no-zero from tiancaiamao/time-format

fmt.Fprintf(buf, "%02d", t%12)
}
case 'l':
fmt.Fprintf(buf, "%d", (t.Time.Hour()%12)+1)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why need we mod 12 here?

case 'f':
fmt.Fprintf(buf, "%06d", t.Time.Microsecond())
case 'U', 'u', 'V', 'v':
// TODO: Fix here.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fix what?

Copy link
Contributor Author

@tiancaiamao tiancaiamao Dec 12, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TODO: Fix here. is a special comment I use, so grep can find it easily.
mysql may use Sunday or Monday as the first day of week, U u V v controls which.

but Go always use Sunday as the first day of week, so not easy to do it now, I leave a TODO.

if m == 0 || m > 12 {
return errors.Trace(ErrInvalidTimeFormat)
}
buf.WriteString(abbrevMonthName[m-1])
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can it use MonthNames[m-1][:3]? Then no need abbrevMonthName.

continue
}

// it's not in pattern match now
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

s/it/It
add .

Conflicts:
	util/types/fsp.go
	util/types/mytime.go
	util/types/time.go
	util/types/time_test.go
@tiancaiamao
Copy link
Contributor Author

PTAL @hanfei1991 @zimulala

@tiancaiamao tiancaiamao changed the base branch from tiancaiamao/timestamp-no-zero to master December 12, 2016 09:38
@coocood
Copy link
Member

coocood commented Dec 12, 2016

LGTM

@@ -322,9 +321,14 @@ func (s *testTimeSuite) getLocation(c *C) *time.Location {

func (s *testTimeSuite) TestCodec(c *C) {
defer testleak.AfterTest(c)()
t, err := ParseTimestamp("2010-10-10 10:11:11")
// mysql timestamp value don't allow month=0 or day=0.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

s/mysql/MySQL

@zimulala
Copy link
Contributor

LGTM

@tiancaiamao tiancaiamao merged commit c30849a into master Dec 12, 2016
@tiancaiamao tiancaiamao deleted the tiancaiamao/time-format branch December 12, 2016 12:31
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.

4 participants