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

Improve type annotations #30

Closed
wants to merge 4 commits into from

Conversation

fransik
Copy link
Contributor

@fransik fransik commented Nov 30, 2020

Hi Matthias!

I came across your ORM after reading about it in Advanced Web Application Architecture (great book btw!). I decided to try it out in a sample project, and so far it's working really well, thank you! 👍

The only issue that I had with it is that phpstorm & phpstan complained about the incorrect type being returned by AggregateRepository::getById() when using it in my repository implementation.

This PR fixes that, and while I was at it, I added some more type annotations so that it plays nicer with both phpstorm and phpstan (and potentially psalm?).

Let me know what you think :)

@matthiasnoback
Copy link
Owner

Thanks, that makes a lot of sense. Can you take a look at the failing analysis?

@fransik
Copy link
Contributor Author

fransik commented Dec 2, 2020

Sure no worries. I had a look and fixed the issues:

  1. The PHPStan version used in CI didn't understand the PHPdoc syntax I used. This was due to Travis pulling the talisorm/php72 and talisorm/phpstan images from the docker registry, but these images are two years old (and thus containing an old version of PHPStan).
    I changed the travis config to do a docker-compose build instead of pull. However it might be better to push the updated images to docker registry and change it back to pull again. CI builds will be faster that way.

  2. Scrutinizer also didn't understand the PHPdoc syntax completely. The ones it didn't understand I prefixed with phpstan- so that PHPStan is able to use the advanced syntax, other tools can fallback to the regular syntax.

@matthiasnoback
Copy link
Owner

Nice work! I'll take a closer look ASAP. Do you have experience with Psalm as well, would it make sense to also add annotations that it can work with?

@fransik
Copy link
Contributor Author

fransik commented Dec 3, 2020

Thanks! I will have a play around with Psalm this weekend, see how it reacts :)

@matthiasnoback
Copy link
Owner

I've merged your work, thanks a lot!
In #31 I reverted the call to docker-compose pull because I have updated the images as you suggested.
Adding Psalm annotations will be nice, but there's no hurry and with your PR you have already added a lot of value to this project. Cool!

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