Skip to content

Add GitLab Statistics #4384

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

Closed
wants to merge 8 commits into from
Closed

Add GitLab Statistics #4384

wants to merge 8 commits into from

Conversation

am-on
Copy link

@am-on am-on commented Jul 28, 2018

Added star and fork statistics on detailed project view for projects with an
valid GitLab URL.

Closes #4301
Ref: #4301

@am-on
Copy link
Author

am-on commented Jul 28, 2018

image

@am-on am-on force-pushed the 4301-add-gitlab-stat branch from 0647ada to c893fdf Compare July 28, 2018 15:10
@@ -413,6 +413,18 @@ def github_repo_info_url(self):
user_name, repo_name = segments[:2]
return f"https://github.com/api/repos/{user_name}/{repo_name}"

@property
def gitlab_repo_info_url(self):
# import pdb; pdb.set_trace()
Copy link
Member

Choose a reason for hiding this comment

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

Comment left behind here.

@nlhkabu
Copy link
Contributor

nlhkabu commented Jul 28, 2018

As discussed at the sprints with @am-on, I would like to know if it's possible to use a macro for the links, e.g. moving this code into a macro:

<a class="vertical-tabs__tab vertical-tabs__tab--with-icon vertical-tabs__tab--condensed gitlab-repo-info__item"
   data-key="html_url" data-attr="href" data-supplement="/network" rel="noopener"
   target="_blank">
  <i class="fa fa-code-branch" aria-hidden="true"></i>
  <strong>Forks: </strong>
  <span class="gitlab-repo-info__item" data-key="forks_count"></span>
</a>

@di
Copy link
Member

di commented Jul 28, 2018

We could also probably make that section more generic, since it's unlikely that a project has both GitHub and gitlab links.

Also, we should probably prefer one over the other here so we don't show approximately the same section twice (I'd lean towards preferring GitHub).

@am-on am-on changed the title Add GitLab Statistics [WIP] Add GitLab Statistics Jul 28, 2018
@am-on am-on force-pushed the 4301-add-gitlab-stat branch 2 times, most recently from 6e3ebc0 to 178c0bc Compare July 28, 2018 20:25
@am-on am-on changed the title [WIP] Add GitLab Statistics Add GitLab Statistics Jul 28, 2018
Copy link
Member

@di di left a comment

Choose a reason for hiding this comment

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

LGTM aside from my question about the CSS

@nlhkabu
Copy link
Contributor

nlhkabu commented Jul 29, 2018

As discussed in the sprints, we're going to create a new SCSS component for these links as vertical-tabs is probably not the best fit for this use case :)

@nlhkabu
Copy link
Contributor

nlhkabu commented Jul 29, 2018

@am-on - what project were you testing this with in development?

@am-on
Copy link
Author

am-on commented Jul 29, 2018

@nlhkabu not sure which is gitlab and which github

pymagnitude
krangpower

@paternal
Copy link

paternal commented Aug 2, 2018

This is nice. 😄

However, Gitlab being free, there are several instances (for instance, my projects are hosted at Framagit). While having warehouse check a list of every single public gitlab instance is a bad idea, I wonder if it would be possible to:

  1. Check if the project homepage URL is a gitlab instance;
  2. If so, fetch and display its statistics.

Thanks for your work!

am-on and others added 6 commits November 18, 2018 09:38
Added star and fork statistics on detailed project view for projects with an
valid GitLab URL.

Ref: pypi#4301
GitLab has no view for stars, so it shouldn't be presented as
clickable link. We are prefering GitHub over GitLab. Because of that
we can generalize class names as only one of them gets rendered.
@nlhkabu nlhkabu force-pushed the 4301-add-gitlab-stat branch from cc024e4 to 5d9e21c Compare November 18, 2018 13:57
@nlhkabu
Copy link
Contributor

nlhkabu commented Nov 18, 2018

Thanks again for this PR @am-on - and sorry for the delay on getting back to it!

I've just pushed a PR that separates out the CSS classes from the JS class (the JS hook is now js-repo-data).

I've also updated the template to be (what I think is) more readable, and adapted the SCSS accordingly.

screenshot from 2018-11-18 13-55-58

screenshot from 2018-11-18 13-55-08

@yeraydiazdiaz would you be able to re-review the JS for this PR? There is an empty catch function at the bottom of repository-info.js that I am not sure about :)

Copy link
Contributor

@yeraydiazdiaz yeraydiazdiaz left a comment

Choose a reason for hiding this comment

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

Thanks for your PR @am-on, @nlhkabu the JS changes look ok on their own, but I feel it could be handled nicely in a Stimulus controller, we could tackle that in a separate PR.

I left a minor comment on the Python side, not a blocker but a nice to have if @am-on is up for it 👍

parsed.netloc == "gitlab.com" or parsed.netloc == "www.gitlab.com"
) and len(segments) >= 2:
user_name, repo_name = segments[:2]
return f"https://gitlab.com/api/v4/projects/{user_name}%2F{repo_name}"
Copy link
Contributor

Choose a reason for hiding this comment

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

this property has a lot of common code with the above. It's not a blocker for the PR, but it'd be nice if we could refactor the common logic into a helper function.

@brainwane
Copy link
Contributor

@am-on would you be interested in finishing this up?

Also, some of us will be at the PyCon North America sprints again this year, May 6th-9th, in case you would like to join us then!

@nlhkabu nlhkabu added the UX/UI design, user experience, user interface label May 20, 2019
@brainwane
Copy link
Contributor

@am-on Is this ready to be re-reviewed?

@nlhkabu
Copy link
Contributor

nlhkabu commented Jun 22, 2019

I believe this is waiting on another HTML and SCSS review on my side (and possible updates).

@nlhkabu nlhkabu self-assigned this Jun 22, 2019
@nlhkabu
Copy link
Contributor

nlhkabu commented Sep 15, 2019

Note:
If / when this PR is re-reviewed, we will need to ensure that the template is appropriately translated.

@yeraydiazdiaz
Copy link
Contributor

@am-on would you be interested in continuing work on this? Looks like it was pretty close to merging.

Base automatically changed from master to main January 21, 2021 18:39
@miketheman
Copy link
Member

#Triage
Hey @am-on ! It's been a couple of years since, and we're curious if you're still interested in working on this?

Some recent changes to the code would need to be resolved, as the Javascript logic has moved as of #12666

@miketheman miketheman added the awaiting-response PRs and issues that are awaiting author response label Dec 16, 2022
@am-on am-on requested a review from a team as a code owner February 22, 2024 19:07
@miketheman
Copy link
Member

This is still. a good idea, but due to age, too many conflicts, closing this PR. If there's still interest, please revive.

@miketheman miketheman closed this Feb 23, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
awaiting-response PRs and issues that are awaiting author response UX/UI design, user experience, user interface
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants