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

fix timezone bug when persisting *gtime.Time to db #1714 #1729

Merged
merged 1 commit into from
May 9, 2022
Merged

fix timezone bug when persisting *gtime.Time to db #1714 #1729

merged 1 commit into from
May 9, 2022

Conversation

wesleywu
Copy link
Contributor

@wesleywu wesleywu commented Apr 3, 2022

No description provided.

@codecov-commenter
Copy link

codecov-commenter commented Apr 3, 2022

Codecov Report

Merging #1729 (1a271ce) into master (52d8371) will decrease coverage by 0.00%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #1729      +/-   ##
==========================================
- Coverage   71.27%   71.27%   -0.01%     
==========================================
  Files         452      452              
  Lines       43310    43310              
==========================================
- Hits        30869    30868       -1     
- Misses      10466    10469       +3     
+ Partials     1975     1973       -2     
Flag Coverage Δ
go-1.15 71.24% <100.00%> (-0.01%) ⬇️
go-1.16 71.23% <100.00%> (+<0.01%) ⬆️
go-1.17 71.23% <100.00%> (+0.03%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
database/gdb/gdb_func.go 86.15% <100.00%> (ø)
os/gspath/gspath_cache.go 87.71% <0.00%> (-7.02%) ⬇️
os/glog/glog_logger_rotate.go 67.31% <0.00%> (ø)
database/gdb/gdb_driver_mysql.go 64.86% <0.00%> (+2.70%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 52d8371...1a271ce. Read the comment docs.

@gqcn
Copy link
Member

gqcn commented Apr 7, 2022

@wesleywu If you need auto time converting feature of underlying driver, please use time.Time instead of gtime.Time.

@gqcn gqcn closed this Apr 7, 2022
@gqcn gqcn added the rejected The proposal or PR is not accepted, which might be conflicted with our design or plan. label Apr 7, 2022
@wesleywu
Copy link
Contributor Author

wesleywu commented Apr 7, 2022

@wesleywu If you need auto time converting feature of underlying driver, please use time.Time instead of gtime.Time.
@gqcn
If I use *time.Time for columns, I could not retrieve them right because issue #1727.
Not considering to use time.Time because I need to check if column value is nil.

Thanks.

@wesleywu
Copy link
Contributor Author

wesleywu commented Apr 7, 2022

@wesleywu If you need auto time converting feature of underlying driver, please use time.Time instead of gtime.Time.

@gqcn
And I don't think I'm using any auto time conversion because my fix did not do any conversion, which should be expected behavior for db persisting. On the contrary, the original implementation used time conversion by calling gtime.Time.String().

@wesleywu
Copy link
Contributor Author

wesleywu commented Apr 7, 2022

the gtime.Time.String() method is flawed because it loses timezone info.

@gqcn gqcn reopened this Apr 12, 2022
@gqcn gqcn removed the rejected The proposal or PR is not accepted, which might be conflicted with our design or plan. label Apr 12, 2022
@gqcn gqcn self-assigned this Apr 12, 2022
@gqcn gqcn added the wip label Apr 12, 2022
@gqcn gqcn merged commit e81d6a8 into gogf:master May 9, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants