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

Use local git repository to get git diffs #153

Merged
merged 56 commits into from
Apr 23, 2021
Merged

Conversation

gudmdharalds
Copy link
Contributor

@gudmdharalds gudmdharalds commented Mar 24, 2021

Let vipgoci_github_diffs_fetch() use local git repository for its source of git diff results instead of using the GitHub API. Rename the function to indicate that it uses the local git-repository as well. Add unit-tests as needed and update older ones.

This will solve #135 .

TODO:

  • Fix remaining bug (see here)
  • Alter MiscPatchChangedLinesTest unit-test to use the vip-go-ci-tests repository, see here.
  • Split vipgoci_github_diffs_fetch() into two: cached and uncached versions
  • Rename function vipgoci_github_diffs_fetch() to vipgoci_gitrepo_diffs_fetch() and move to git-repo.php.
  • Fix issues noted in comments.
  • Add unit-tests for uncached version of diff function:
    • Add files
    • Add file, with permissions
    • Add content to existing files
    • Remove content from existing files
    • Rename files
    • Remove files
    • Change permissions of file
    • One test that covers all of the above
    • Add file that start with the name a/ and b/
  • Update README
  • Detect version of git used, exit when minimum is not met
  • Functions should return more information and statistics:
    • vipgoci_gitrepo_diffs_fetch_uncached() -- overall statistics on changes, previous name of files renamed
    • vipgoci_gitrepo_diffs_fetch() -- overall statistics on changes, takes effects of filter into account
  • Run full-unit tests
  • Check automated unit-tests

Copy link
Member

@ariskataoka ariskataoka left a comment

Choose a reason for hiding this comment

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

I've left a few comments. Mostly to improve a bit of readability and code reuse. Let me know what do you think!

ap-file-types.php Show resolved Hide resolved
github-api.php Outdated Show resolved Hide resolved
github-api.php Outdated Show resolved Hide resolved
github-api.php Outdated Show resolved Hide resolved
github-api.php Outdated Show resolved Hide resolved
github-api.php Outdated Show resolved Hide resolved
Copy link
Member

@ariskataoka ariskataoka left a comment

Choose a reason for hiding this comment

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

Just minor comments

function vipgoci_git_version(): ?string {
static $git_version_cached = null;

if ( null !== $git_version_cached ) {
Copy link
Member

Choose a reason for hiding this comment

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

Early returns <3

string $commit_id_b
): array {

vipgoci_log(
Copy link
Member

Choose a reason for hiding this comment

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

I think these tabs and break lines were unintentional?

Copy link
Contributor Author

@gudmdharalds gudmdharalds Apr 14, 2021

Choose a reason for hiding this comment

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

Resolved in d1471e7

I'm assuming you are referring to line 779 (now removed). The tabs on line 781, 782 and so forth are intentional to make the code easier to read.

git-repo.php Show resolved Hide resolved
git-repo.php Outdated Show resolved Hide resolved
Copy link
Collaborator

@wpcomvip-vipgoci-bot wpcomvip-vipgoci-bot left a comment

Choose a reason for hiding this comment

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

phpcs scanning turned up:

⚠️ 1 warning


This bot provides automated PHP Linting and PHPCS scanning, read more here.

git-repo.php Outdated
'commit_id_b' => $commit_id_b,
)
);
$test = '';
Copy link
Collaborator

Choose a reason for hiding this comment

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

⚠️ Warning( severity 5 ): Unused variable $test (VariableAnalysis.CodeAnalysis.VariableAnalysis.UnusedVariable).

@gudmdharalds gudmdharalds merged commit 61f087f into master Apr 23, 2021
@gudmdharalds gudmdharalds deleted the use-git-diffs branch April 30, 2021 15:20
@gudmdharalds gudmdharalds added this to the 1.0.0 milestone May 11, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants