Refactor SecToTime() function #18863

schorsch13 · 2022-02-23T10:06:23Z

Changes

Add helper method to reduce redundancy
Expand the scope from displaying days to years
Reduce irrelevance by not displaying small units (hours, minutes, seconds) when bigger ones apply (years)

schorsch13 · 2022-02-23T12:59:39Z

I would require some help from the maintainers. The build is failing due to linting errors in modules/utils/sec_to_time.go on line 53. I am unable to find any problems there and I also tried both the golangci-lint and the gofumpt linter and both throw the same error locally. I tried linting the file with both of them but the error is still there.

modules/util/sec_to_time_test.go

silverwind · 2022-02-23T15:02:19Z

Try running make fmt.

schorsch13 · 2022-02-23T15:28:39Z

Try running make fmt.

I tried but after running git status

On branch refactor/milestone-update-time
Your branch is up-to-date with 'origin/refactor/milestone-update-time'.

Changes not staged for commit:
  (use "git add <file>..." to update what will be committed)
  (use "git restore <file>..." to discard changes in working directory)
	modified:   models/issue_xref.go
	modified:   models/migrations/v210.go
	modified:   models/migrations/v210_test.go
	modified:   modules/indexer/code/bleve.go
	modified:   modules/notification/action/action.go
	modified:   modules/notification/indexer/indexer.go
	modified:   modules/notification/mail/mail.go
	modified:   modules/notification/notification.go
	modified:   modules/notification/ui/ui.go
	modified:   modules/notification/webhook/webhook.go
	modified:   modules/structs/repo.go
	modified:   routers/web/repo/branch.go
	modified:   services/mailer/mail.go
	modified:   services/release/release.go

no changes added to commit (use "git add" and/or "git commit -a")

The file modules/util/sec_to_time.go has not changed

techknowlogick · 2022-02-23T16:06:42Z

@schorsch13 there are times when golang differences produce different results when using go fmt, are you using go1.17?

silverwind · 2022-02-23T16:11:28Z

By the way, there is a mechanism for relative dates in widespread use but with less precision (renders 7 days ago), this one is only used on a handful of pages. Do we really need two ways to render relative dates? What's the benefit of having this one?

schorsch13 · 2022-02-23T17:07:52Z

@schorsch13 there are times when golang differences produce different results when using go fmt, are you using go1.17?

go version go1.17.7 linux/amd64

schorsch13 · 2022-02-23T17:08:47Z

By the way, there is a mechanism for relative dates in widespread use but with less precision (renders 7 days ago), this one is only used on a handful of pages. Do we really need two ways to render relative dates? What's the benefit of having this one?

I am not aware that there is another function. I started tweaking this method after realizing that the elapsed time of milestones is being displayed in hours and I thought like WTF why

silverwind · 2022-02-23T17:16:21Z

I think it's timeutil.TimeSince and the milestone usecase may even be a match for it.

schorsch13 · 2022-02-23T17:42:37Z

If I get this correctly the modules/timeutil/since.go class does display years, months, weeks, days, hours, minutes and seconds all in one (sinc_test.go). My implementation cuts off all smaller units (hours) if bigger ones (years) are present

silverwind · 2022-02-23T18:50:50Z

Regarding formatting, also try go install mvdan.cc/gofumpt@latest and then make fmt.

schorsch13 · 2022-02-23T18:55:22Z

go install mvdan.cc/gofumpt@latest

doesnt change anything. could you try if formatting on your local machine actually does smth to the modules/util/sec_to_time.go file

schorsch13 · 2022-02-23T19:00:18Z

So far I tried formatting on my laptop (manjaro linux) and on my desktop (linux mint)

silverwind · 2022-02-23T19:44:25Z

It does produce a sizable diff, even in files not edited by this PR:

Diff

diff --git a/models/issue_xref.go b/models/issue_xref.go
index fdabedf29..7b2f097c1 100644
--- a/models/issue_xref.go
+++ b/models/issue_xref.go
@@ -194,9 +194,10 @@ func (issue *Issue) updateCrossReferenceList(list []*crossReference, xref *cross
 }

 // verifyReferencedIssue will check if the referenced issue exists, and whether the doer has permission to do what
 func (issue *Issue) verifyReferencedIssue(stdCtx context.Context, ctx *crossReferencesContext, repo *repo_model.Repository,
-       ref references.IssueReference) (*Issue, references.XRefAction, error) {
+       ref references.IssueReference,
+) (*Issue, references.XRefAction, error) {
        refIssue := &Issue{RepoID: repo.ID, Index: ref.Index}
        refAction := ref.Action
        e := db.GetEngine(stdCtx)

diff --git a/models/migrations/v210.go b/models/migrations/v210.go
index cf50760b9..9da8ca9db 100644
--- a/models/migrations/v210.go
+++ b/models/migrations/v210.go
@@ -10,10 +10,10 @@ import (
        "fmt"
        "strings"

        "code.gitea.io/gitea/modules/timeutil"
+
        "github.com/tstranex/u2f"
-
        "xorm.io/xorm"
        "xorm.io/xorm/schemas"
 )

diff --git a/models/migrations/v210_test.go b/models/migrations/v210_test.go
index 3e10b3ce8..70dbe61b0 100644
--- a/models/migrations/v210_test.go
+++ b/models/migrations/v210_test.go
@@ -7,8 +7,9 @@ package migrations
 import (
        "testing"

        "code.gitea.io/gitea/modules/timeutil"
+
        "github.com/stretchr/testify/assert"
        "xorm.io/xorm/schemas"
 )

diff --git a/modules/indexer/code/bleve.go b/modules/indexer/code/bleve.go
index 281c14c11..309b33bed 100644
--- a/modules/indexer/code/bleve.go
+++ b/modules/indexer/code/bleve.go
@@ -181,9 +181,10 @@ func NewBleveIndexer(indexDir string) (*BleveIndexer, bool, error) {
        return indexer, created, err
 }

 func (b *BleveIndexer) addUpdate(ctx context.Context, batchWriter git.WriteCloserError, batchReader *bufio.Reader, commitSha string,
-       update fileUpdate, repo *repo_model.Repository, batch *gitea_bleve.FlushingBatch) error {
+       update fileUpdate, repo *repo_model.Repository, batch *gitea_bleve.FlushingBatch,
+) error {
        // Ignore vendored files in code search
        if setting.Indexer.ExcludeVendored && analyze.IsVendor(update.Filename) {
                return nil
        }
diff --git a/modules/notification/action/action.go b/modules/notification/action/action.go
index ed4ce3dd1..bab28db47 100644
--- a/modules/notification/action/action.go
+++ b/modules/notification/action/action.go
@@ -90,9 +90,10 @@ func (a *actionNotifier) NotifyIssueChangeStatus(doer *user_model.User, issue *m
 }

 // NotifyCreateIssueComment notifies comment on an issue to notifiers
 func (a *actionNotifier) NotifyCreateIssueComment(doer *user_model.User, repo *repo_model.Repository,
-       issue *models.Issue, comment *models.Comment, mentions []*user_model.User) {
+       issue *models.Issue, comment *models.Comment, mentions []*user_model.User,
+) {
        act := &models.Action{
                ActUserID: doer.ID,
                ActUser:   doer,
                RepoID:    issue.Repo.ID,
diff --git a/modules/notification/indexer/indexer.go b/modules/notification/indexer/indexer.go
index 26f19e779..48a491f3f 100644
--- a/modules/notification/indexer/indexer.go
+++ b/modules/notification/indexer/indexer.go
@@ -29,9 +29,10 @@ func NewNotifier() base.Notifier {
        return &indexerNotifier{}
 }

 func (r *indexerNotifier) NotifyCreateIssueComment(doer *user_model.User, repo *repo_model.Repository,
-       issue *models.Issue, comment *models.Comment, mentions []*user_model.User) {
+       issue *models.Issue, comment *models.Comment, mentions []*user_model.User,
+) {
        if comment.Type == models.CommentTypeComment {
                if issue.Comments == nil {
                        if err := issue.LoadDiscussComments(); err != nil {
                                log.Error("LoadComments failed: %v", err)
diff --git a/modules/notification/mail/mail.go b/modules/notification/mail/mail.go
index 4ce2726fc..b74482fbf 100644
--- a/modules/notification/mail/mail.go
+++ b/modules/notification/mail/mail.go
@@ -28,9 +28,10 @@ func NewNotifier() base.Notifier {
        return &mailNotifier{}
 }

 func (m *mailNotifier) NotifyCreateIssueComment(doer *user_model.User, repo *repo_model.Repository,
-       issue *models.Issue, comment *models.Comment, mentions []*user_model.User) {
+       issue *models.Issue, comment *models.Comment, mentions []*user_model.User,
+) {
        ctx, _, finished := process.GetManager().AddContext(graceful.GetManager().HammerContext(), fmt.Sprintf("mailNotifier.NotifyCreateIssueComment Issue[%d] #%d in [%d]", issue.ID, issue.Index, issue.RepoID))
        defer finished()

        var act models.ActionType
diff --git a/modules/notification/notification.go b/modules/notification/notification.go
index a0acd0156..e8d5d07b3 100644
--- a/modules/notification/notification.go
+++ b/modules/notification/notification.go
@@ -38,9 +38,10 @@ func NewContext() {
 }

 // NotifyCreateIssueComment notifies issue comment related message to notifiers
 func NotifyCreateIssueComment(doer *user_model.User, repo *repo_model.Repository,
-       issue *models.Issue, comment *models.Comment, mentions []*user_model.User) {
+       issue *models.Issue, comment *models.Comment, mentions []*user_model.User,
+) {
        for _, notifier := range notifiers {
                notifier.NotifyCreateIssueComment(doer, repo, issue, comment, mentions)
        }
 }
@@ -200,9 +201,10 @@ func NotifyIssueChangeRef(doer *user_model.User, issue *models.Issue, oldRef str
 }

 // NotifyIssueChangeLabels notifies change labels to notifiers
 func NotifyIssueChangeLabels(doer *user_model.User, issue *models.Issue,
-       addedLabels, removedLabels []*models.Label) {
+       addedLabels, removedLabels []*models.Label,
+) {
        for _, notifier := range notifiers {
                notifier.NotifyIssueChangeLabels(doer, issue, addedLabels, removedLabels)
        }
 }
diff --git a/modules/notification/ui/ui.go b/modules/notification/ui/ui.go
index 7f6bfa398..037167f64 100644
--- a/modules/notification/ui/ui.go
+++ b/modules/notification/ui/ui.go
@@ -52,9 +52,10 @@ func (ns *notificationService) Run() {
        graceful.GetManager().RunWithShutdownFns(ns.issueQueue.Run)
 }

 func (ns *notificationService) NotifyCreateIssueComment(doer *user_model.User, repo *repo_model.Repository,
-       issue *models.Issue, comment *models.Comment, mentions []*user_model.User) {
+       issue *models.Issue, comment *models.Comment, mentions []*user_model.User,
+) {
        opts := issueNotificationOpts{
                IssueID:              issue.ID,
                NotificationAuthorID: doer.ID,
        }
diff --git a/modules/notification/webhook/webhook.go b/modules/notification/webhook/webhook.go
index ea70faa3c..d4d5eea6c 100644
--- a/modules/notification/webhook/webhook.go
+++ b/modules/notification/webhook/webhook.go
@@ -423,9 +423,10 @@ func (m *webhookNotifier) NotifyUpdateComment(doer *user_model.User, c *models.C
        }
 }

 func (m *webhookNotifier) NotifyCreateIssueComment(doer *user_model.User, repo *repo_model.Repository,
-       issue *models.Issue, comment *models.Comment, mentions []*user_model.User) {
+       issue *models.Issue, comment *models.Comment, mentions []*user_model.User,
+) {
        mode, _ := models.AccessLevel(doer, repo)

        var err error
        if issue.IsPull {
@@ -497,9 +498,10 @@ func (m *webhookNotifier) NotifyDeleteComment(doer *user_model.User, comment *mo
        }
 }

 func (m *webhookNotifier) NotifyIssueChangeLabels(doer *user_model.User, issue *models.Issue,
-       addedLabels, removedLabels []*models.Label) {
+       addedLabels, removedLabels []*models.Label,
+) {
        ctx, _, finished := process.GetManager().AddContext(graceful.GetManager().HammerContext(), fmt.Sprintf("webhook.NotifyIssueChangeLabels User: %s[%d] Issue[%d] #%d in [%d]", doer.Name, doer.ID, issue.ID, issue.Index, issue.RepoID))
        defer finished()

        var err error
diff --git a/modules/structs/repo.go b/modules/structs/repo.go
index 5a1e99e36..087ae941f 100644
--- a/modules/structs/repo.go
+++ b/modules/structs/repo.go
@@ -219,9 +219,8 @@ type GenerateRepoOption struct {

 // CreateBranchRepoOption options when creating a branch in a repository
 // swagger:model
 type CreateBranchRepoOption struct {
-
        // Name of the branch to create
        //
        // required: true
        // unique: true
diff --git a/routers/web/repo/branch.go b/routers/web/repo/branch.go
index 5d19fd118..489ef9a35 100644
--- a/routers/web/repo/branch.go
+++ b/routers/web/repo/branch.go
@@ -232,9 +232,10 @@ func loadBranches(ctx *context.Context, skip, limit int) (*Branch, []*Branch, in
 }

 func loadOneBranch(ctx *context.Context, rawBranch, defaultBranch *git.Branch, protectedBranches []*models.ProtectedBranch,
        repoIDToRepo map[int64]*repo_model.Repository,
-       repoIDToGitRepo map[int64]*git.Repository) *Branch {
+       repoIDToGitRepo map[int64]*git.Repository,
+) *Branch {
        log.Trace("loadOneBranch: '%s'", rawBranch.Name)

        commit, err := rawBranch.GetCommit()
        if err != nil {
diff --git a/services/mailer/mail.go b/services/mailer/mail.go
index d5f3f2ac0..3983237fc 100644
--- a/services/mailer/mail.go
+++ b/services/mailer/mail.go
@@ -426,9 +426,10 @@ func SendIssueAssignedMail(issue *models.Issue, doer *user_model.User, content s

 // actionToTemplate returns the type and name of the action facing the user
 // (slightly different from models.ActionType) and the name of the template to use (based on availability)
 func actionToTemplate(issue *models.Issue, actionType models.ActionType,
-       commentType models.CommentType, reviewType models.ReviewType) (typeName, name, template string) {
+       commentType models.CommentType, reviewType models.ReviewType,
+) (typeName, name, template string) {
        if issue.IsPull {
                typeName = "pull"
        } else {
                typeName = "issue"
diff --git a/services/release/release.go b/services/release/release.go
index 9e3ea3370..0df863523 100644
--- a/services/release/release.go
+++ b/services/release/release.go
@@ -185,9 +185,10 @@ func CreateNewTag(ctx context.Context, doer *user_model.User, repo *repo_model.R
 // addAttachmentUUIDs accept a slice of new created attachments' uuids which will be reassigned release_id as the created release
 // delAttachmentUUIDs accept a slice of attachments' uuids which will be deleted from the release
 // editAttachments accept a map of attachment uuid to new attachment name which will be updated with attachments.
 func UpdateRelease(doer *user_model.User, gitRepo *git.Repository, rel *models.Release,
-       addAttachmentUUIDs, delAttachmentUUIDs []string, editAttachments map[string]string) (err error) {
+       addAttachmentUUIDs, delAttachmentUUIDs []string, editAttachments map[string]string,
+) (err error) {
        if rel.ID == 0 {
                return errors.New("UpdateRelease only accepts an exist release")
        }
        isCreated, err := createTag(gitRepo, rel, "")

So I the issue is CI installs gofumpt@latest which is v0.3.0 released yesterday and which presumably changed the formatting again since the last release. Filed #18866 to lock down its version. After that PR has landed, your PR should come out clean I think.

schorsch13 · 2022-02-23T19:50:31Z

Alright thank you very much.

There is still the question open regarding whether the util/sec_to_time class is necessary if there is timeutil/since

silverwind · 2022-02-23T21:27:02Z

I haven't studied it in detail, but I ought to say a single function should suffice and we display 7 days ago instead of 7 days 15 hours 6 minutes ago. Or does it make any sense to have such precision? Does timeutil/since have an option to increase precision when needed?

codecov-commenter · 2022-02-24T08:15:35Z

Codecov Report

Merging #18863 (62e4424) into main (4d93984) will decrease coverage by 0.02%.
The diff coverage is 44.89%.

@@            Coverage Diff             @@
##             main   #18863      +/-   ##
==========================================
- Coverage   46.64%   46.62%   -0.03%     
==========================================
  Files         846      853       +7     
  Lines      121331   122521    +1190     
==========================================
+ Hits        56595    57125     +530     
- Misses      57859    58507     +648     
- Partials     6877     6889      +12

Impacted Files	Coverage Δ
cmd/admin_auth_ldap.go	`79.59% <ø> (ø)`
models/auth/webauthn.go	`43.11% <ø> (ø)`
models/org.go	`73.88% <0.00%> (+0.21%)`	⬆️
modules/git/pipeline/lfs_nogogit.go	`0.00% <0.00%> (ø)`
modules/git/pipeline/namerev.go	`0.00% <0.00%> (ø)`
modules/graceful/manager.go	`22.16% <0.00%> (-0.13%)`	⬇️
modules/graceful/manager_unix.go	`38.88% <0.00%> (-0.95%)`	⬇️
modules/queue/manager.go	`39.59% <0.00%> (-0.83%)`	⬇️
modules/queue/queue_bytefifo.go	`52.72% <0.00%> (+3.28%)`	⬆️
modules/templates/helper.go	`51.57% <ø> (ø)`
... and 111 more

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 2b5e013...62e4424. Read the comment docs.

silverwind

I guess it's a nice refactor, thought I think we should eventually just merge with TimeSince.

modules/util/sec_to_time.go

lunny · 2022-02-25T06:11:57Z

How to translate the English words?

schorsch13 · 2022-02-25T06:27:06Z

How to translate the English words?

do you mean years, days, etc.?

silverwind · 2022-02-25T08:43:03Z

BTW, I imagine there should be 3rd-party packages that implement such time formatting and i'd prefer that instead of having to maintain this in the codebase.

lunny · 2022-02-26T02:25:13Z

How to translate the English words?

do you mean years, days, etc.?

Yes.

modules/util/sec_to_time.go

schorsch13 · 2022-02-28T21:04:45Z

@6543 I tried building the latest commit locally and it succeeds... I cant find out why the CI fails

* giteaofficial/main: [API] Allow removing issues (go-gitea#18879) Refactor SecToTime() function (go-gitea#18863) Improve mirror iterator (go-gitea#18928) Fix login with email panic when email is not exist (go-gitea#18941)

- Add helper method to reduce redundancy - Expand the scope from displaying days to years - Reduce irrelevance by not displaying small units (hours, minutes, seconds) when bigger ones apply (years)

GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Feb 23, 2022

silverwind reviewed Feb 23, 2022

View reviewed changes

modules/util/sec_to_time_test.go Outdated Show resolved Hide resolved

Add weeks

3149be7

zeripath closed this Feb 23, 2022

zeripath reopened this Feb 23, 2022

Update months to be 30 days instead of 4 weeks

2a0c27f

schorsch13 force-pushed the refactor/milestone-update-time branch from ced0cba to 5d827c0 Compare February 24, 2022 07:23

Update tests to match new time format

62e4424

schorsch13 force-pushed the refactor/milestone-update-time branch from 5d827c0 to 62e4424 Compare February 24, 2022 07:54

Merge branch 'main' into refactor/milestone-update-time

f266b7a

schorsch13 requested a review from silverwind February 24, 2022 19:28

silverwind approved these changes Feb 24, 2022

View reviewed changes

GiteaBot added lgtm/need 1 This PR needs approval from one additional maintainer to be merged. and removed lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. labels Feb 24, 2022

silverwind reviewed Feb 24, 2022

View reviewed changes

modules/util/sec_to_time.go Show resolved Hide resolved

lunny added the type/refactoring Existing code has been cleaned up. There should be no new functionality. label Feb 25, 2022

Gusted added this to the 1.17.0 milestone Feb 28, 2022

Gusted approved these changes Feb 28, 2022

View reviewed changes

GiteaBot added lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. and removed lgtm/need 1 This PR needs approval from one additional maintainer to be merged. labels Feb 28, 2022

6543 reviewed Feb 28, 2022

View reviewed changes

modules/util/sec_to_time.go Outdated Show resolved Hide resolved

6543 reviewed Feb 28, 2022

View reviewed changes

modules/util/sec_to_time.go Outdated Show resolved Hide resolved

Make formatTime private

d697e63

6543 approved these changes Feb 28, 2022

View reviewed changes

Merge branch 'main' into refactor/milestone-update-time

291da7e

6543 merged commit 6859b69 into go-gitea:main Feb 28, 2022

go-gitea locked and limited conversation to collaborators Apr 28, 2022

Uh oh!

Refactor SecToTime() function #18863

Refactor SecToTime() function #18863

Uh oh!

Conversation

schorsch13 commented Feb 23, 2022

Changes

Uh oh!

schorsch13 commented Feb 23, 2022

Uh oh!

Uh oh!

silverwind commented Feb 23, 2022

Uh oh!

schorsch13 commented Feb 23, 2022

Uh oh!

techknowlogick commented Feb 23, 2022

Uh oh!

silverwind commented Feb 23, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

schorsch13 commented Feb 23, 2022

Uh oh!

schorsch13 commented Feb 23, 2022

Uh oh!

silverwind commented Feb 23, 2022

Uh oh!

schorsch13 commented Feb 23, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

silverwind commented Feb 23, 2022

Uh oh!

schorsch13 commented Feb 23, 2022

Uh oh!

schorsch13 commented Feb 23, 2022

Uh oh!

silverwind commented Feb 23, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

schorsch13 commented Feb 23, 2022

Uh oh!

silverwind commented Feb 23, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov-commenter commented Feb 24, 2022

Codecov Report

Uh oh!

silverwind left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

lunny commented Feb 25, 2022

Uh oh!

schorsch13 commented Feb 25, 2022

Uh oh!

silverwind commented Feb 25, 2022

Uh oh!

lunny commented Feb 26, 2022

Uh oh!

Uh oh!

Uh oh!

schorsch13 commented Feb 28, 2022

Uh oh!

Uh oh!

silverwind commented Feb 23, 2022 •

edited

Loading

schorsch13 commented Feb 23, 2022 •

edited

Loading

silverwind commented Feb 23, 2022 •

edited

Loading

silverwind commented Feb 23, 2022 •

edited

Loading