Skip to content

Conversation

123kiran17
Copy link
Contributor

psalm errors are solved

Copy link
Collaborator

@transistive transistive left a comment

Choose a reason for hiding this comment

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

Good work!

Small remarks: if most of the methods need to be marked with @api, it is probably worth it to mark the class as @api instead of every method.

Also, before I can merge this PR you have to resolve the conflicts.

The easiest way is to merge main into this branch and resolve the conflicts using the tool in phpstorm

composer.json Outdated
"ext-json": "*",
"php": "^8.1"
"php": "^8.1",
"symfony/finder": "^7.2"
Copy link
Collaborator

Choose a reason for hiding this comment

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

this should be a development dependency, if we even need it.

* @api
*/

public function getType(): ?string
Copy link
Collaborator

Choose a reason for hiding this comment

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

we should look into declaring the class as part of the api instead of all the methods, what do you think?

// $username = 'neo4j';
// $password = '9lWmptqBgxBOz8NVcTJjgs3cHPyYmsy63ui6Spmw1d0';
//// $connectionUrl = 'https://6f72daa1.databases.neo4j.io/db/neo4j/query/v2';

Copy link
Collaborator

Choose a reason for hiding this comment

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

This definitely should be removed!

/**
* @template TKey of array-key
* @template TValue
* @implements IteratorAggregate<TKey, TValue>
Copy link
Collaborator

Choose a reason for hiding this comment

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

Great stuff! Glad you werre able to figure this one out

@transistive transistive merged commit 8d3af70 into main Jan 17, 2025
1 check 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.

2 participants