-
Notifications
You must be signed in to change notification settings - Fork 6
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
Add option to order results by last commit date #37
base: master
Are you sure you want to change the base?
Conversation
Travis tests have failedHey @CLiu13, 1st Buildcoala --non-interactive -V
TravisBuddy Request Identifier: 2c64fc40-eac6-11e8-8440-8fa58700f614 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add tests to keep the percentage above 50%
6a3f1df
to
c8ebd92
Compare
Travis tests have failedHey @CLiu13, 1st Buildcoala --non-interactive -V
TravisBuddy Request Identifier: 4d29d910-eaf7-11e8-8440-8fa58700f614 |
Travis tests have failedHey @CLiu13, 1st Buildcoala --non-interactive -V
TravisBuddy Request Identifier: c1a72ae0-eaf7-11e8-8440-8fa58700f614 |
Travis tests have failedHey @CLiu13, 1st Buildpytest
TravisBuddy Request Identifier: 40d80cd0-eaf8-11e8-8440-8fa58700f614 |
Travis tests have failedHey @CLiu13, 1st Buildpytest
TravisBuddy Request Identifier: 31a58ed0-eaf9-11e8-8440-8fa58700f614 |
Travis tests have failedHey @CLiu13, 1st Buildpytest
TravisBuddy Request Identifier: 1a8bc010-eafa-11e8-8440-8fa58700f614 |
Travis tests have failedHey @CLiu13, 1st Buildcoala --non-interactive -V
TravisBuddy Request Identifier: c3c73bf0-ebcc-11e8-aa09-4ba844902889 |
Travis tests have failedHey @CLiu13, 1st Buildcoala --non-interactive -V
TravisBuddy Request Identifier: 9f1581d0-ebcd-11e8-aa09-4ba844902889 |
Travis tests have failedHey @CLiu13, 1st Buildpytest
TravisBuddy Request Identifier: 8f42a520-ebce-11e8-aa09-4ba844902889 |
This adds the ability to clone every repo (which can be used for other purposes later) and determine the date of the last commit, which is then recorded and can be used to sort the export results. Closes ksdme#7
|
||
def _get_last_commit_date(self): | ||
repo_commit_dates = [''] | ||
for commit in self.commits: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So, this will not work with --use-repo-list
mode from #34, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why iterate over every commit?
That is horribly inefficient.
Any list of commits should be already pre-sorted, most recent first, so you should be able to get only the first using next(iter(commit_list))
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If there is a problem in the GitLabRepository implementation, that needs to be solved upstream in IGitt.
If so, add a comment here in this PR explaining the problem, and we'll check and you can create an issue and solve the IGitt bug.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, the list of commits is not pre-sorted.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, we'll get IGitt fixed so this doesnt need to be so ugly.
@@ -21,6 +22,9 @@ def __init__(self, token, group, **kargs): | |||
self._token = GitHubToken(token) | |||
self._org = GitHubOrganization(self._token, self._group) | |||
|
|||
for repo in self._org.repositories: | |||
repo.get_last_commit_date = MethodType(_get_last_commit_date, repo) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't like how you are patching in a method here. We already have support for Gitman and once your #34 gets merged we will support --use-repo-list
. This method will work when the merge happens.
At this point of time, we have two ways to address this problem.
- Provide a static method on
OrgHost
to get the commit dates of a given repo independently. - We already support tools like Gitman, we can use them to clone the repos and get commit dates from the local copy.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jayvdb Your thoughts?
|
||
def _get_last_commit_date(self): | ||
repo_commit_dates = [''] | ||
for commit in self.commits: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If there is a problem in the GitLabRepository implementation, that needs to be solved upstream in IGitt.
If so, add a comment here in this PR explaining the problem, and we'll check and you can create an issue and solve the IGitt bug.
The current approach to this issue involves getting a list of commits from Gitlab by using Igitt. This list of commits is unsorted due to Igitt's implementation of this, which returns the list of commits as a set, causing it to be randomly shuffled. |
This adds the ability to clone every repo (which
can be used for other purposes later) and determine
the date of the last commit, which is then recorded
and can be used to sort the export results.
Closes #7