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

Add type annotations #30

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

DivineDominion
Copy link

This PR adds type annotations esp. to the Document type so that in apps, tools have an easier time figuring out that $document->getYAML() ?? [] is valid.

At least in some cases -- I was surprised that string is also possible! So good to know, need to work around that.

  • Honestly, I'm not sure how to deal with this w.r.t. releasing new versions of the package and making sure that the minimum PHP version is installed to support these annotations :)
  • The test case function return type annotations of void were added to comply with tools to check consistency (phpstan) mostly; I don't feel strongly about adding or keeeping them out either way. Can revert that commit if you want.

@mnapoli
Copy link
Owner

mnapoli commented Jun 30, 2024

Tests are failing, we could remove support for PHP 7.4 if that helps?

@DivineDominion
Copy link
Author

Ugh, of course I just ran the tests locally and didn't think of CI and older PHP's!

@mnapoli What's a sensibel schedule to drop support for older language versions in the PHP community? I can't help, I'm new :)

@mnapoli
Copy link
Owner

mnapoli commented Jul 2, 2024

7.4 is ancient, no problem at all to remove support from it. 8.0 is possible too.

@DivineDominion
Copy link
Author

@mnapoli Could you rerun tests with PHP 8 to check? I tested with PHP 8.2 and 8.3

@mnapoli
Copy link
Owner

mnapoli commented Jul 4, 2024

Even though the build was canceled it seems the tests passed:

Screen-001872

@DivineDominion
Copy link
Author

Yeah looks like an odd timeout?

WDYT about a php-8 branch, then I change the merge target to that? Dropping PHP 7.x support, one can probably make other changes while at it

@mnapoli
Copy link
Owner

mnapoli commented Jul 6, 2024

This wasn't a timeout, this is normal the job was cancelled because teh 7.4 job failed.

So we just need to remove 7.4. Can be in another PR or here.

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