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

Git statistics in Activity tab #4724

Merged
merged 10 commits into from May 4, 2019

Conversation

@lafriks
Copy link
Member

commented Aug 15, 2018

Fixes #4697

TODO:

  • Implement required methods to load data
  • Git statistics display in Activity Tab
  • UI cleanup
  • Implement bar chart with top 10 users in UI (can be implemented in other PR)

Screenshot:
attels

@lafriks lafriks added this to the 1.6.0 milestone Aug 15, 2018

@lafriks lafriks added the status/wip label Aug 15, 2018

@techknowlogick techknowlogick modified the milestones: 1.6.0, 1.7.0 Sep 3, 2018

@lunny lunny referenced this pull request Sep 7, 2018

Closed

Add commits activity #4697

@techknowlogick techknowlogick modified the milestones: 1.7.0, 1.8.0 Dec 19, 2018

@stale

This comment has been minimized.

Copy link

commented Feb 17, 2019

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs during the next 2 months. Thank you for your contributions.

@stale stale bot added the stale label Feb 17, 2019

@stale stale bot removed the stale label Feb 17, 2019

@techknowlogick techknowlogick modified the milestones: 1.8.0, 1.9.0 Feb 19, 2019

@lafriks lafriks force-pushed the lafriks-fork:feat/git_stats branch from a8240f8 to afa37b6 Feb 23, 2019

@lafriks lafriks modified the milestones: 1.9.0, 1.8.0 Feb 23, 2019

@lafriks lafriks removed the status/wip label Feb 23, 2019

@codecov-io

This comment has been minimized.

Copy link

commented Feb 23, 2019

Codecov Report

Merging #4724 into master will increase coverage by 0.01%.
The diff coverage is 45%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #4724      +/-   ##
==========================================
+ Coverage   41.23%   41.24%   +0.01%     
==========================================
  Files         421      422       +1     
  Lines       58125    58277     +152     
==========================================
+ Hits        23965    24037      +72     
- Misses      30989    31061      +72     
- Partials     3171     3179       +8
Impacted Files Coverage Δ
models/repo_activity.go 52.21% <10%> (-14.07%) ⬇️
routers/routes/routes.go 82.39% <100%> (+0.09%) ⬆️
routers/repo/activity.go 40.35% <8%> (-24.36%) ⬇️
modules/git/repo_stats.go 84.5% <84.5%> (ø)
models/gpg_key.go 55.83% <0%> (-0.84%) ⬇️
routers/repo/view.go 43.03% <0%> (+1.01%) ⬆️
models/unit.go 67.56% <0%> (+5.4%) ⬆️

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 2933ae4...0d73090. Read the comment docs.

Show resolved Hide resolved models/repo_activity.go Outdated
@sapk
Copy link
Member

left a comment

Works ans looks greats.
Just need to move calls to git command to go-gitea/git as it will ease the migration when fully switching to gopkg.in/src-d/go-git.v4. Another solution is to directly replace those calls by gopkg.in/src-d/go-git.v4.

Show resolved Hide resolved Gopkg.lock Outdated
@lafriks

This comment has been minimized.

Copy link
Member Author

commented Feb 24, 2019

If anyone could point to method to do this with src-d/git I could rewrite this to use it

@lunny

This comment has been minimized.

Copy link
Member

commented Feb 28, 2019

@sapk I think we need to move go-gitea/git back as a module of go-gitea/gitea. But we need keep all git codes as a well-formed sub package.

Show resolved Hide resolved models/repo_activity.go Outdated

@techknowlogick techknowlogick modified the milestones: 1.8.0, 1.9.0 Mar 8, 2019

@lafriks lafriks force-pushed the lafriks-fork:feat/git_stats branch from e20eaec to 5e96ab1 Apr 30, 2019

@lafriks

This comment has been minimized.

Copy link
Member Author

commented Apr 30, 2019

@lunny @sapk please review

@lafriks lafriks added the changelog label Apr 30, 2019

@sapk

sapk approved these changes Apr 30, 2019

@GiteaBot GiteaBot added lgtm/need 1 and removed lgtm/need 2 labels Apr 30, 2019

@sapk

This comment has been minimized.

Copy link
Member

commented Apr 30, 2019

@lafriks LGTM but would be perfect with tests for new code under models and modules/git

@lafriks

This comment has been minimized.

Copy link
Member Author

commented May 1, 2019

@sapk test added

@lunny

This comment has been minimized.

Copy link
Member

commented May 2, 2019

It should be commits.
image

@lunny

This comment has been minimized.

Copy link
Member

commented May 2, 2019

I think your code is right. But when some language's translation is missing, it will be replaced by English. And if on this language it's no difference between singular and plural. Then it will buggy. So we should know the real language before TrN called.

@lafriks

This comment has been minimized.

Copy link
Member Author

commented May 2, 2019

@lunny it is only for untranslated strings for Chinese languages. For other it will be correct, and TrN function can not really know if it is untranslated or not. Anyway it is out of scope for this PR

@lunny

lunny approved these changes May 4, 2019

@GiteaBot GiteaBot added lgtm/done and removed lgtm/need 1 labels May 4, 2019

@lunny

This comment has been minimized.

Copy link
Member

commented May 4, 2019

That is out of this PR but it's still a problem.

@lafriks lafriks merged commit 1fa9662 into go-gitea:master May 4, 2019

2 checks passed

approvals/lgtm this commit looks good
continuous-integration/drone/pr Build is passing
Details

@lafriks lafriks deleted the lafriks-fork:feat/git_stats branch May 4, 2019

@lafriks

This comment has been minimized.

Copy link
Member Author

commented May 4, 2019

@lunny I agree but by currently uses i18n I don't see how that could be fixed

@ngourdon

This comment has been minimized.

Copy link
Contributor

commented May 4, 2019

@lafriks
I cannot make a good translation of the messages in french.
Some parts of the message have singular and plural form (%d author(s), %d commit(s), ...) others don't (has pushed, have changed).

In french, I need to agree the verb with the grammatical number.

  • 1 auteur a poussé
  • 2 auteurs ont poussé

Depending on the verb, I think in english we have to as well.

  • 1 author has pushed
  • 2 authors have pushed
@lafriks

This comment has been minimized.

Copy link
Member Author

commented May 4, 2019

Ok, I will try to split it out a bit more and add additional parts to be translatable depending on count

@mrsdizzie

This comment has been minimized.

Copy link
Contributor

commented May 4, 2019

Maybe the words "has" and "have" could be removed and it would still have the same meaning without the problem above?

ie: 1 author pushed 1 commit, On Master 4 files changed. 2 authors pushed 2 commits ...

Maybe no need for has/have since "pushed" and "changed" already indicate past tense and work for both singular and plural? Not sure if that makes the translation any easier or not...

@ngourdon

This comment has been minimized.

Copy link
Contributor

commented May 4, 2019

@mrsdizzie This will not make it any easier for french.

@lafriks
Only need to split the parts with verbs (has pushed, have changed)
Maybe we can join "%d author(s) pushed" and "%d file(s) changed" because the form of the verb depends on the subject.

@lafriks

This comment has been minimized.

Copy link
Member Author

commented May 4, 2019

@ngourdon I have submitted PR #6848

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
8 participants
You can’t perform that action at this time.