Skip to content

Conversation

@tacman
Copy link
Contributor

@tacman tacman commented Feb 10, 2022

tests aren't working yet.

The current 'master' branch has a minimum stability of 'dev'. I think before this PR is accepted, perhaps you could drop that line, make sure everything runs, then tag is as 1.0, so anyone currently using it has a safe way of keeping it.

Alternatively, maybe create another branch and merge these changes in?

I'm not actually that familiar with how to use tsvector and such, which is why I used this bundle. So if you can fix the tests, I can clean up the code deprecations and such.

Thanks.

@tacman
Copy link
Contributor Author

tacman commented Feb 12, 2022

Hi, James, any chance you can take a look at this? In particular, I don't know why the search test fails, I removed the 2 tests, but obviously that's not the right approach. Thx.


namespace VertigoLabs\DoctrineFullTextPostgres\Common;

use App\Entity\Media;
Copy link
Owner

Choose a reason for hiding this comment

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

This line shouldn't exist

Comment on lines +101 to +110
// $this->checkWatchFields($class, $prop, $attribute);
// $metaData->mapField([
// 'fieldName' => $prop->getName(),
// 'columnName' => $this->getColumnName($prop, $annotation),
// 'type' => 'tsvector',
// 'weight' => strtoupper($annotation->weight),
// 'language' => strtolower($annotation->language),
// 'nullable' => $this->isWatchFieldNullable($class, $annotation)
// ]);

Copy link
Owner

Choose a reason for hiding this comment

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

Commented out code should be removed

foreach ($prop->getAttributes() as $reflectionAttribute) {
if ($reflectionAttribute->getName() === TsVector::class) {
// /** @var TsVector $attribute */
$attribute = new ($reflectionAttribute->getName())(...$reflectionAttribute->getArguments()); // crazy, something is wrong here.
Copy link
Owner

Choose a reason for hiding this comment

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

I'm a little apprehensive about this comment!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No doubt! I'll see if I can figure it out. I'm fighting a little bit with supporting both attributes and annotations, I think that attributes should take priority if both exist.

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.

2 participants