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

#33 Schema generation extensions #47

Open
wants to merge 12 commits into
base: main
Choose a base branch
from

Conversation

Hubbitus
Copy link
Contributor

@Hubbitus Hubbitus commented Jan 8, 2022

Major refactoring with PhpClassPropertyType typing

Closes #33

@Hubbitus Hubbitus force-pushed the issue-33-schema-generation-extensions branch from 5f11244 to 1cc81b7 Compare January 8, 2022 14:23
@Hubbitus
Copy link
Contributor Author

Hubbitus commented Jan 8, 2022

@nick-zh, please look I've fixed almost all tests, but there still 2 failed like:

PhpKafka\PhpAvroSchemaGenerator\Tests\Integration\Parser\ClassParserTest::testGetProperties
ParseError: syntax error, unexpected '|', expecting variable (T_VARIABLE)

/var/www/html/example/classes/SomeTestClass.php:83
/var/www/html/src/Parser/ClassParser.php:221
/var/www/html/src/Parser/ClassParser.php:158
/var/www/html/tests/Integration/Parser/ClassParserTest.php:56
phpvfscomposer:///var/www/html/vendor/phpunit/phpunit/phpunit:97

But that related to the \PhpKafka\PhpAvroSchemaGenerator\Example\SomeTestClass::$blaaaaaaaa declaration:

public int|string $blaaaaaaaa;

That use Union type declaration which is available since PHP 8.0.0.
So they failed expectedly on PHP container based on version 7.4.
Should I upgrade the container to version 8.0.0 or do I just need to disable 2-3 tests? FYI all tests passed on my machine locally with PHP 8.0.2.

@nick-zh
Copy link
Member

nick-zh commented Jan 8, 2022

I think it is ok for 3.x to drop php7.4 support, so you can go ahead and update the container to 8.0

@Hubbitus
Copy link
Contributor Author

Hubbitus commented Jan 9, 2022

All tests passed now. There is coverage below 100% thought. Please say if you are generally willing to accept such PR and I could continue work to extend coverage and deal with some code-style warnings.

Or we wait your refactoring in terms of #49?

@nick-zh
Copy link
Member

nick-zh commented Jan 9, 2022

Yeah might be best to maybe hold of for the moment, since some changes / renaming might complicate things unnecessarily for you. Sry for the inconvenience. I hope i can tackle this next weekend. I already started, but still need to figure out some stuff

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.

Please allow types re-definition (custom mapping)
2 participants