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

Added code that allows user to work with different versions. #116

Merged
merged 13 commits into from
May 20, 2024

Conversation

ulin-evgeny
Copy link
Contributor

No description provided.


namespace RonasIT\Support\Contracts;

interface VersionEnumContract {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
interface VersionEnumContract {
interface VersionEnumContract
{

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

@@ -24,6 +27,40 @@ public function boot()
app(ExcelServiceProvider::class, ['app' => app()])->boot();

$this->loadViewsFrom(__DIR__ . '/Stubs', 'ronasit');

/**
Copy link
Collaborator

Choose a reason for hiding this comment

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

let's move it to the separate function extendRouter

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

Copy link
Collaborator

Choose a reason for hiding this comment

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

please move only router extension to the separate method, not all logic :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

{
$route = Route::getRoutes()->match(request());

return $route->parameters()['version'] ?? str_replace('/v', '', $route->getPrefix());
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
return $route->parameters()['version'] ?? str_replace('/v', '', $route->getPrefix());
return Arr::get($route->parameters(), 'version', str_replace('/v', '', $route->getPrefix());

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

return $route->parameters()['version'] ?? str_replace('/v', '', $route->getPrefix());
}

public static function is(VersionEnumContract $checkedVersion): bool
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
public static function is(VersionEnumContract $checkedVersion): bool
public static function is(VersionEnumContract $expectedVersion): bool

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

{
$version = self::current();

return $version >= $from->value && $version <= $to->value;
Copy link
Collaborator

Choose a reason for hiding this comment

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

please use version_compare native function instead

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

@@ -68,6 +69,13 @@ public function callRawRequest(string $method, string $uri, $content, array $hea
return $this->call($method, $uri, [], [], [], $server, $content);
}

public function json($method, $uri, array $data = [], array $headers = [], ?VersionEnumContract $apiVersion = null): TestResponse
Copy link
Collaborator

Choose a reason for hiding this comment

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

let's keep this code in EmptyProject repository

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

@DenTray DenTray removed their assignment May 2, 2024
@@ -24,6 +27,40 @@ public function boot()
app(ExcelServiceProvider::class, ['app' => app()])->boot();

$this->loadViewsFrom(__DIR__ . '/Stubs', 'ronasit');

/**
Copy link
Collaborator

Choose a reason for hiding this comment

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

please move only router extension to the separate method, not all logic :)

tests/TestCase.php Outdated Show resolved Hide resolved
tests/VersionTest.php Outdated Show resolved Hide resolved
tests/VersionTest.php Outdated Show resolved Hide resolved
tests/VersionTest.php Outdated Show resolved Hide resolved
Copy link
Collaborator

@DenTray DenTray left a comment

Choose a reason for hiding this comment

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

please increase test coverage

@DenTray DenTray removed their assignment May 14, 2024
@@ -0,0 +1,23 @@
<?php

namespace RonasIT\Support\Enum;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
namespace RonasIT\Support\Enum;
namespace RonasIT\Support\Tests\Support\Enum;

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

src/HelpersServiceProvider.php Show resolved Hide resolved

use RonasIT\Support\Contracts\VersionEnumContract;

class VersionEnum implements VersionEnumContract
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
class VersionEnum implements VersionEnumContract
// Backward compatibility with PHP < 8
class VersionEnum implements VersionEnumContract

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

Comment on lines 77 to 79
Route::macro('whereIn', function ($parameters, array $values) {
return $this->assignExpressionToParameters($parameters, implode('|', $values));
});
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
Route::macro('whereIn', function ($parameters, array $values) {
return $this->assignExpressionToParameters($parameters, implode('|', $values));
});
Route::macro('whereIn', fn ($parameters, array $values) => $this->assignExpressionToParameters($parameters, implode('|', $values)));

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

Comment on lines 82 to 85
return $this->where(collect(Arr::wrap($parameters))
->mapWithKeys(function ($parameter) use ($expression) {
return [$parameter => $expression];
})->all());
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
return $this->where(collect(Arr::wrap($parameters))
->mapWithKeys(function ($parameter) use ($expression) {
return [$parameter => $expression];
})->all());
return $this
->where(collect(Arr::wrap($parameters))
->mapWithKeys(fn ($parameter) => [$parameter => $expression])
->all());

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

Comment on lines 24 to 32
$versionFrom = $this->createMock(VersionEnumContract::class);
$versionFrom->value = VersionEnum::v1;
$versionTo = $this->createMock(VersionEnumContract::class);
$versionTo->value = VersionEnum::v2;
Route::group(['prefix' => 'v{version}'], function () use ($versionTo, $versionFrom) {
Route::get(static::ROUTE_OBJECT, function () {
return 'Route';
})->versionRange($versionFrom, $versionTo);
});
Copy link
Collaborator

Choose a reason for hiding this comment

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

let's move it to the separate methods of the new trait RouteMockTrait

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

Comment on lines 34 to 44
$versionFrom = $this->createMock(VersionEnumContract::class);
$versionFrom->value = VersionEnum::v11;
$versionTo = $this->createMock(VersionEnumContract::class);
$versionTo->value = VersionEnum::v12;
Route::group(['prefix' => 'v{version}'], function () use ($versionTo, $versionFrom) {
RouteFacade::versionRange($versionFrom, $versionTo)->group(function () {
Route::get(static::ROUTE_FACADE, function () {
return 'RouteFacade';
});
});
});
Copy link
Collaborator

Choose a reason for hiding this comment

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

et's move it to the separate method of the new trait RouteMockTrait

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

/**
* @dataProvider getTestVersionRangeData
*/
public function testVersionRange(string $version, bool $assert, string $route): void
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
public function testVersionRange(string $version, bool $assert, string $route): void
public function testVersionRange(string $version, bool $isCorrectVersion, string $route): void

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

{
$response = $this->get("/v{$version}{$route}");

$status = $assert ? 200 : 404;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
$status = $assert ? 200 : 404;
$status = ($assert) ? 200 : 404;

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

return [
[
'version' => '1',
'assert' => true,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
'assert' => true,
'is_correct_version' => true,

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.


$this->app->bind(VersionEnumContract::class, VersionEnum::class);

switch ($this->getProvidedData()['route']) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

please move it to the RouteMockTrait

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

public function getTestVersionRangeData(): array
{
return [
// Range
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
// Range

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

'route' => static::ROUTE_FACADE_RANGE,
],

// From
Copy link
Collaborator

Choose a reason for hiding this comment

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

please implement several tests instead of comments

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

@ulin-evgeny ulin-evgeny removed their assignment May 17, 2024
tests/VersionRouteTest.php Outdated Show resolved Hide resolved
tests/VersionRouteTest.php Outdated Show resolved Hide resolved
@DenTray DenTray merged commit 878cdba into master May 20, 2024
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants