Skip to content

Commit

Permalink
Fix mistake
Browse files Browse the repository at this point in the history
  • Loading branch information
flugg committed Jan 23, 2018
1 parent fb07af3 commit 44e6cd5
Show file tree
Hide file tree
Showing 4 changed files with 45 additions and 41 deletions.
4 changes: 2 additions & 2 deletions composer.json
Original file line number Diff line number Diff line change
Expand Up @@ -20,8 +20,8 @@
],
"require": {
"php": "^7.0",
"illuminate/contracts": "5.1.* || 5.2.* || 5.3.* || 5.4.* || 5.5.*",
"illuminate/support": "5.1.* || 5.2.* || 5.3.* || 5.4.* || 5.5.*",
"illuminate/contracts": "5.1.* || 5.2.* || 5.3.* || 5.4.* || 5.5.* || 5.6.*",
"illuminate/support": "5.1.* || 5.2.* || 5.3.* || 5.4.* || 5.5.* || 5.6.*",
"league/fractal": "^0.16.0"
},
"require-dev": {
Expand Down
1 change: 0 additions & 1 deletion src/Http/Responses/Decorators/PrettyPrintDecorator.php
Original file line number Diff line number Diff line change
Expand Up @@ -30,4 +30,3 @@ public function make(array $data, int $status, array $headers = []): JsonRespons
return $response;
}
}
}
20 changes: 8 additions & 12 deletions src/TransformBuilder.php
Original file line number Diff line number Diff line change
Expand Up @@ -272,7 +272,8 @@ protected function includeTransformerRelations(BaseTransformer $transformer)
$this->with(Collection::make($transformer->defaultRelations())
->filter(function ($constrain, $relation) use ($relations) {
return ! in_array(is_numeric($relation) ? $constrain : $relation, $relations);
})->all());
})
->all());
}

/**
Expand All @@ -289,29 +290,24 @@ protected function stripEagerLoadConstraints(array $relations): array
}

/**
* Remove parameters from relations that must be eager loaded and
* filter out the relations which have an includeXxx method.
* Remove parameters from relations that must be eager loaded and filter
* out the relations which have an includeXxx method.
*
* @param array $relations
* @param \Flugg\Responder\Transformers\Transformer|callable|string|null $transformer
* @param array $relations
* @param \Flugg\Responder\Transformers\Transformer|callable|string|null $transformer
* @return array
*/
protected function prepareEagerLoadableRelations(array $relations, $transformer): array
{
$cleanedRelations = [];

foreach ($relations as $key => $value) {
// Strip out parameters from relation name

This comment has been minimized.

Copy link
@IlCallo

IlCallo Jan 26, 2018

Contributor

To me, it doesn't seem a good idea to strip away all comments...
The code becomes a lot more difficult to work on if you have to guess what code does and why it does it in that way

This comment has been minimized.

Copy link
@flugg

flugg Jan 26, 2018

Author Owner

While I don’t disagree with you, I want to be consistent with the look of the code, and instead of having inline comments spread around, I’m trying to instead refactor code that’s hard to understand into smaller, more easily understood methods and use the docblock comments for additional information.

The code above is getting a heavy makeover though for next release, together with the relationship code in the transformer. A lot of that code was starting to become pretty confusing.

Oh, and I’m almost finished with the integration test suite!

This comment has been minimized.

Copy link
@IlCallo

IlCallo Jan 26, 2018

Contributor

Well, I think that inline comments and docblocks have very different purpose: implementation insight are not meant to be put into docblocks, which are read and processed by the IDE and shown to the developers using the library.
Laravel use both inline comments and docblocks too, but I won't discuss further this thing, mine was just a suggestion: you decide how to manage your code obviously :)

This comment has been minimized.

Copy link
@flugg

flugg Jan 26, 2018

Author Owner

Yeah, you’re absolutely right. This is mostly to satisfy my own OCD than anything else haha. However, the amount of inline comments in the Laravel source code is pretty limited, because most of the code reads like a book and the comments become superfluous. This is the goal I’m reaching for, even if it might be a bit optimistic. The important thing for me is to stay consistent and sadly I’m not an expert at making every comment 3 characters shorter than the previous line 😄 (although, I did manage for the configuration file!)

I appreciate the feedback though and will keep it in mind for the future.

$relationName = explode(':', is_numeric($key) ? $value : $key)[0];
// Ignores all relation which have a includeXxx method
// method_exists does not care if the $transformer is actually an object or not
if (method_exists($transformer, 'include' . ucfirst($relationName))) {
continue;
}

// If the key is numeric, value is the relation name: return it
// Otherwise the key is the relation name and the value is a custom scope:
// return the relation with the value untouched
if(is_numeric($key)) {
if (is_numeric($key)) {
$cleanedRelations[$key] = $relationName;
} else {
$cleanedRelations[$relationName] = $value;
Expand Down
61 changes: 35 additions & 26 deletions tests/Unit/TransformBuilderTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -327,14 +327,8 @@ public function testTransformMethodExtractsAndEagerLoadsRelationsWhenThereAreRel
$this->resource->shouldReceive('getTransformer')->andReturn($transformer = Mockery::mock(Transformer::class));
$transformer->shouldReceive('defaultRelations')->andReturn([]);

$this->builder->resource()->with(['foo:first(aa|bb)', 'bar:second(cc|dd)' => function() {}])->transform();

// Model should receive the relations names without parameters,
// while the transformFactory should receive also parameters to let Fractal use them
// We must use the Mockery::on() method because with() method will try to do a strict match
// for the closure resulting in a failure, because it will check
// if it's the same closure reference but no closure are alike, even when they are defined identically.
// Here we just check that 'bar' element contains a closure.
$this->builder->resource()->with(['foo:first(aa|bb)', 'bar:second(cc|dd)' => function () { }])->transform();

$model->shouldHaveReceived('load')->with(Mockery::on(function (array $relations) {
return ($relations[0] == 'foo') && ($relations['bar'] instanceof \Closure);
}))->once();
Expand All @@ -346,15 +340,44 @@ public function testTransformMethodExtractsAndEagerLoadsRelationsWhenThereAreRel
}

/**
* Assert that the [transform] method do not eager load relations for which is present an include method.
* Assert that the [transform] method doesn't eager load relations not present in the $relations list.
*/
/*public function testTransformMethodDoesntEagerLoadNonListedRelations()
{
$this->transformFactory->shouldReceive('make')->andReturn([]);
$this->resource->shouldReceive('getData')->andReturn($model = Mockery::mock(Model::class));
$model->shouldReceive('load')->andReturnSelf();
$this->resource->shouldReceive('getTransformer')->andReturn($transformer = new class extends Transformer
{
protected $relations = ['foo'];
});
$this->builder->resource()->with(['foo', 'bar'])->transform();
$model->shouldHaveReceived('load')->with(['foo'])->once();
$this->transformFactory->shouldHaveReceived('make')->with($this->resource, $this->serializer, [
'includes' => ['foo'],
'excludes' => [],
'fieldsets' => [],
])->once();
}*/

/**
* Assert that the [transform] method doesn't eager load relations which has an include method.
*/
public function testTransformMethodDoNotEagerLoadsRelationsForWhichAnIncludeMethodExists()
public function testTransformMethodDoesntEagerLoadRelationsWithAnIncludeMethod()
{
$this->transformFactory->shouldReceive('make')->andReturn([]);
$this->resource->shouldReceive('getData')->andReturn($model = Mockery::mock(Model::class));
$model->shouldReceive('load')->andReturnSelf();
// It's not possible to easily mock method_exists with mockery so we must rely on a stub
$this->resource->shouldReceive('getTransformer')->andReturn(new TransformerWithIncludeMethods());
$this->resource->shouldReceive('getTransformer')->andReturn(new class extends Transformer
{
protected $relations = ['foo', 'bar'];
protected $load = ['baz'];

public function includeBar() { }
public function includeBaz() { }
});

$this->builder->resource()->with($relations = ['foo', 'bar'])->transform();

Expand Down Expand Up @@ -399,18 +422,4 @@ public function testOnlyMethodAllowsMultipleCallsAndStrings()
'fieldsets' => ['foo', 'bar', 'baz'],
])->once();
}
}

class TransformerWithIncludeMethods extends Transformer {
protected $relations = ['foo', 'bar'];

protected $load = ['baz'];

public function includeBar() {
//
}

public function includeBaz() {
//
}
}

0 comments on commit 44e6cd5

Please sign in to comment.