Skip to content

Added type annotation to all methods #203

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 10 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
84 changes: 14 additions & 70 deletions .github/workflows/tests.yml
Original file line number Diff line number Diff line change
Expand Up @@ -2,42 +2,23 @@ name: Tests

on:
push:
branches: [ main ]
pull_request:

jobs:
tests:
name: PHP ${{ matrix.php }}; Symfony ${{ matrix.symfony }}
runs-on: ubuntu-20.04

name: Test PHP ${{ matrix.php-version }} ${{ matrix.name }}
runs-on: ubuntu-latest
strategy:
fail-fast: false
matrix:
php: ['5.6', '7.0', '7.1', '7.2', '7.3', '7.4', '8.0', '8.1']
symfony: ['3', '4', '5', '6']
exclude:
- php: '5.6'
symfony: '4'
- php: '5.6'
symfony: '5'
- php: '5.6'
symfony: '6'
- php: '7.0'
symfony: '4'
- php: '7.0'
symfony: '5'
- php: '7.0'
symfony: '6'
- php: '7.1'
symfony: '5'
- php: '7.1'
symfony: '6'
- php: '7.2'
symfony: '6'
- php: '7.3'
symfony: '6'
- php: '7.4'
symfony: '6'
- php: '8.1'
symfony: '3'
php-version: ['8.1', '8.2']
composer-flags: ['']
name: ['']
include:
- php: 8.0
composer-flags: '--prefer-lowest'
name: '(prefer lowest dependencies)'

steps:
- name: Checkout Code
Expand All @@ -47,50 +28,13 @@ jobs:
uses: shivammathur/setup-php@v2
with:
php-version: ${{ matrix.php }}
tools: composer:v2
coverage: none

- name: Setup Problem Matchers
run: echo "::add-matcher::${{ runner.tool_cache }}/phpunit.json"

- name: Select Symfony 3
uses: nick-invision/retry@v1
with:
timeout_minutes: 5
max_attempts: 5
command: composer require "symfony/process:^3.4" --no-update --no-interaction
if: "matrix.symfony == '3'"

- name: Select Symfony 4
uses: nick-invision/retry@v1
with:
timeout_minutes: 5
max_attempts: 5
command: composer require "symfony/process:^4.4" --no-update --no-interaction
if: "matrix.symfony == '4'"

- name: Select Symfony 5
uses: nick-invision/retry@v1
with:
timeout_minutes: 5
max_attempts: 5
command: composer require "symfony/process:^5.3" --no-update --no-interaction
if: "matrix.symfony == '5'"

- name: Select Symfony 6
uses: nick-invision/retry@v1
with:
timeout_minutes: 5
max_attempts: 5
command: composer require "symfony/process:^6.0" --no-update --no-interaction
if: "matrix.symfony == '6'"

- name: Install PHP Dependencies
uses: nick-invision/retry@v1
with:
timeout_minutes: 5
max_attempts: 5
command: composer update --no-interaction --no-progress
- name: Install Composer dependencies
run: |
composer update --prefer-dist --no-interaction ${{ matrix.composer-flags }}

- name: Execute PHPUnit
run: vendor/bin/phpunit
15 changes: 6 additions & 9 deletions composer.json
Original file line number Diff line number Diff line change
Expand Up @@ -35,23 +35,20 @@
}
},
"require": {
"php": "^5.6 || ^7.0 || ^8.0",
"php": "^8.0",
"ext-pcre": "*",
"symfony/polyfill-mbstring": "^1.7",
"symfony/process": "^3.4 || ^4.4 || ^5.0 || ^6.0"
"symfony/process": "^5.4 || ^6.0"
},
"require-dev": {
"ext-fileinfo": "*",
"phpspec/prophecy": "^1.10.2",
"phpunit/phpunit": "^5.7.27 || ^6.5.14 || ^7.5.20 || ^8.5.20 || ^9.5.9",
"phpspec/prophecy-phpunit": "^2.0",
"phpunit/phpunit": "^7.5.20 || ^8.5.20 || ^9.5.9",
"psr/log": "^1.0"
},
"suggest": {
"ext-fileinfo": "Required to determine the mimetype of a blob",
"psr/log": "Required to use loggers for reporting of execution"
},
"config": {
"preferred-install": "dist"
"preferred-install": "dist",
"sort-packages": true
},
"minimum-stability": "dev",
"prefer-stable": true
Expand Down
10 changes: 9 additions & 1 deletion src/Gitonomy/Git/Admin.php
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@ class Admin
* @param bool $bare indicate to create a bare repository
* @param array $options options for Repository creation
*
* @throws RuntimeException Directory exists or not writable (only if debug=true)
* @throws RuntimeException Directory exists or not writable
*
* @return Repository
*/
Expand Down Expand Up @@ -151,6 +151,8 @@ public static function mirrorTo($path, $url, array $options = [])
* @param array $args arguments to be added to the command-line
* @param array $options options for Repository creation
*
* @throws RuntimeException Error while initializing repository
*
* @return Repository
*/
public static function cloneRepository($path, $url, array $args = [], array $options = [])
Expand All @@ -168,6 +170,12 @@ public static function cloneRepository($path, $url, array $args = [], array $opt

/**
* This internal method is used to create a process object.
*
* @param string $command
* @param array $args
* @param array $options
*
* @return Process
Comment on lines +174 to +178
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 this is not the way to go. Instead we should add really PHP type.
It's easily doable for argument since PHP allow it in a BC manner.
But the return type will be a huge BC break!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@lyrixx Could you please elaborate on what you mean by BC?

You mean instead of

     * @param string $command
     * @param array  $args
     * @param array  $options
     *
     * @return Process
     private static function getProcess($command, array $args = [], array $options = [])

one should write

     * @param string $command
     * @param array  $args
     * @param array  $options
     private static function getProcess($command, array $args = [], array $options = []): Process

I'm all for it but I thought this wouldn't work with PHP 5.6. If it does work - it won't work with mixed types at least.

If this is ok, could I go one step further and add PHP typing for all the arguments as well to ditch the DocBlocks entirely?

Copy link
Member

@lyrixx lyrixx May 22, 2023

Choose a reason for hiding this comment

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

BC mean Backward Compatible. see https://symfony.com/bc ; We should use the same policy.

The patch would be:

    /**
     * This internal method is used to create a process object.
     *
     * @return Process
     */
    private static function getProcess(string $command, array $args = [], array $options = [])

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh I see, I misunderstood you completely the first time.
So if I understood you correctly this time then I should add typed arguments instead of DocBlocks, but leave the Return in the DocBlocks?

I haven't worked much where BC was needed, so its better I'll ask you.
Is this case also valid when writing instead of this

    /**
     * @param Commit | string $hash
     *
     * @return Tag[] An array of Tag objects
     */
    public function resolveTags($hash)

this

    /**
     * @return Tag[] An array of Tag objects
     */
    public function resolveTags(Commit | string $hash)

Copy link
Member

Choose a reason for hiding this comment

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

Unfortunately, this project is still compatible with PHP 5.6 😨 ! So wee cannot write this code (union). But with PHP 8+, you can 👍🏼

I'll bump this and I'll do a new release (next week I guess)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Perfect. Should I change everything that's not a union-type to php types instead of DocBlocks though like you required?

Copy link
Member

Choose a reason for hiding this comment

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

yes, please :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I just figured out that primitive type-hints weren't supported until PHP 7.0.
So all I can do is add native type-hints only for arrays and objects.

Copy link
Member

Choose a reason for hiding this comment

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

can you wait a bit? I'll bump some things...

Copy link
Member

Choose a reason for hiding this comment

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

see #204 🎉

*/
private static function getProcess($command, array $args = [], array $options = [])
{
Expand Down
17 changes: 14 additions & 3 deletions src/Gitonomy/Git/Blame.php
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@

use Gitonomy\Git\Blame\Line;
use Gitonomy\Git\Exception\InvalidArgumentException;
use Gitonomy\Git\Exception\ProcessException;
use Gitonomy\Git\Parser\BlameParser;

/**
Expand Down Expand Up @@ -47,9 +48,12 @@ class Blame implements \Countable
protected $lines;

/**
* @param string $lineRange Argument to pass to git blame (-L).
* Can be a line range (40,60 or 40,+21)
* or a regexp ('/^function$/')
* @param Repository $repository
* @param Revision $revision
* @param string $file
* @param string $lineRange Argument to pass to git blame (-L).
* Can be a line range (40,60 or 40,+21)
* or a regexp ('/^function$/')
*/
public function __construct(Repository $repository, Revision $revision, $file, $lineRange = null)
{
Expand All @@ -60,6 +64,10 @@ public function __construct(Repository $repository, Revision $revision, $file, $
}

/**
* @param int $number Line number
*
* @throws InvalidArgumentException Line number does either not exist or is below 1
*
* @return Line
*/
public function getLine($number)
Expand Down Expand Up @@ -108,6 +116,9 @@ public function getGroupedLines()
}

/**
* @throws ProcessException Error while executing git command (debug-mode only)
* or when there are Problems with executing the Process
*
* @return Line[] All lines of the blame.
*/
public function getLines()
Expand Down
5 changes: 4 additions & 1 deletion src/Gitonomy/Git/Blob.php
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,8 @@

namespace Gitonomy\Git;

use Gitonomy\Git\Exception\ProcessException;

/**
* Representation of a Blob commit.
*
Expand Down Expand Up @@ -58,7 +60,8 @@ public function getHash()
}

/**
* @throws ProcessException Error occurred while getting content of blob
* @throws ProcessException Error while executing git command (debug-mode only)
* or when there are Problems with executing the Process
*
* @return string Content of the blob.
*/
Expand Down
29 changes: 29 additions & 0 deletions src/Gitonomy/Git/Commit.php
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,9 @@ class Commit extends Revision
*
* @param Repository $repository Repository of the commit
* @param string $hash Hash of the commit
* @param array $data Associative array of commit data.
*
* @throws ReferenceNotFoundException Hash not matching regular expression
*/
public function __construct(Repository $repository, $hash, array $data = [])
{
Expand All @@ -50,6 +53,9 @@ public function __construct(Repository $repository, $hash, array $data = [])
$this->setData($data);
}

/**
* @param array $data Associative array of commit data.
*/
public function setData(array $data)
{
foreach ($data as $name => $value) {
Expand All @@ -58,6 +64,9 @@ public function setData(array $data)
}

/**
* @throws ProcessException Error while executing git command (debug-mode only)
* or when there are Problems with executing the Process
*
* @return Diff
*/
public function getDiff()
Expand Down Expand Up @@ -144,6 +153,11 @@ public function getTree()
}

/**
* @param string $path
*
* @throws ProcessException Error while executing git command (debug-mode only)
* or when there are Problems with executing the Process
*
* @return Commit
*/
public function getLastModification($path = null)
Expand Down Expand Up @@ -203,6 +217,8 @@ public function resolveReferences()
* @param bool $local set true to try to locate a commit on local repository
* @param bool $remote set true to try to locate a commit on remote repository
*
* @throws InvalidArgumentException You should a least set one argument to true
*
* @return Reference[]|Branch[] An array of Reference\Branch
*/
public function getIncludingBranches($local = true, $remote = true)
Expand Down Expand Up @@ -347,6 +363,19 @@ public function getCommit()
return $this;
}

/**
* Possible options are: treeHash, parentHashes, authorName, authorEmail, authorDate,
* committerName, committerEmail, committerDate, message or an other attribute.
*
* @param string $name
*
* @throws ProcessException Error while executing git command (debug-mode only)
* or when there are Problems with executing the Process
* @throws ReferenceNotFoundException Reference not found
* @throws InvalidArgumentException No data with give name
*
* @return Tree|string
*/
private function getData($name)
{
if (isset($this->data[$name])) {
Expand Down
3 changes: 3 additions & 0 deletions src/Gitonomy/Git/CommitReference.php
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,9 @@ class CommitReference
*/
private $hash;

/**
* @param string $hash
*/
public function __construct($hash)
{
$this->hash = $hash;
Expand Down
3 changes: 3 additions & 0 deletions src/Gitonomy/Git/Diff/Diff.php
Original file line number Diff line number Diff line change
Expand Up @@ -55,6 +55,9 @@ public static function parse($rawDiff)
return new self($parser->files, $rawDiff);
}

/**
* @param Repository $repository
*/
public function setRepository(Repository $repository)
{
foreach ($this->files as $file) {
Expand Down
Loading