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 phpstan-return annotation to translateText #41

Merged
merged 1 commit into from
Feb 16, 2024

Conversation

VincentLanglet
Copy link
Contributor

Hi @daniel-jones-deepl,

When using static analysis tool like PHPStan or Psalm, it is possible to explain that the return type of translateText

  • is a TextResult when the input is a string
  • is an array when the input is an array

Cf https://phpstan.org/blog/phpstan-1-6-0-with-conditional-return-types#conditional-return-types

This would be helpful to help the tool understanding better the result type and avoid useless extra checks.

@JanEbbing
Copy link
Member

Sure, we have this in python already. I'll review this today.

@JanEbbing
Copy link
Member

LGTM

@JanEbbing JanEbbing merged commit cc4e3d8 into DeepLcom:main Feb 16, 2024
@VincentLanglet
Copy link
Contributor Author

Thanks. Do you think a new tag would be possible ? Or is one planned soon ?

@JanEbbing
Copy link
Member

Yes, I will make a new release, I had some issue with it on friday

@JanEbbing
Copy link
Member

Hi @VincentLanglet , sorry I just noticed a new release would include release for Arabic in the new Version. Is this urgent or can it wait a few days?

@VincentLanglet
Copy link
Contributor Author

Hi @VincentLanglet , sorry I just noticed a new release would include release for Arabic in the new Version. Is this urgent or can it wait a few days?

Hi @JanEbbing , it's not urgent.

Also maybe the new release could include a fix for #43 (comment) if the issue is on this library side.

@JanEbbing
Copy link
Member

I think that issue is API-side. I remember a case where a Dotnet user had the same problem for a different text - ill report it to the responsible team, but not sure this can be fixed quickly.

@JanEbbing
Copy link
Member

@VincentLanglet released in v1.7.0 now

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.

3 participants