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 phpstub for amqp extension (php-amqp) #11172

Merged
merged 1 commit into from
Dec 10, 2024

Conversation

mkilmanas
Copy link
Contributor

@mkilmanas mkilmanas commented Nov 27, 2024

Disclaimer: first time contributor here

This adds phpstub file for AMQP extension by copying contents of their originally provided stubs and concatenating into a single file.

What I am uncertain of and would like your feedback on:

  • Should stubs in psalm contain original docblocks with explanations? Or only those with annotations that help with types, deprecations, etc?
  • I have added a link to their LICENCE file (copying the whole file over seems a bit of an overkill). Is that sufficient?
  • I have found two places to register/allow the new extension - have I missed any others? It did not seem like any tests would be specifically checking for each extension, so no new tests added either.

Looking forward to your comments (and hopefully a merge at the end)

@mkilmanas
Copy link
Contributor Author

Hey, what's the status and overall position with this PR - is it at all worth pursuing further?

2 checks failed for debatable reasons:
a) The PR does not have any labels assigned, which I am not in position to add myself, so it's up for maintainers, I suppose
b) Backwards compatibility check is failing, but I find the actual reason to be extremely formal and cannot think of any real-life situation where that could actually break anything or cause any other kind of trouble.

Please let me know what you think and anything that I could improve here.

@danog danog added the release:feature The PR will be included in 'Features' section of the release notes label Dec 10, 2024
@danog danog merged commit 3a4b0e8 into vimeo:5.x Dec 10, 2024
48 of 50 checks passed
@danog
Copy link
Collaborator

danog commented Dec 10, 2024

Thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release:feature The PR will be included in 'Features' section of the release notes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants