Skip to content

[Translation] Added Crowdin Translation Provider #40947

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
May 9, 2021
Merged

[Translation] Added Crowdin Translation Provider #40947

merged 1 commit into from
May 9, 2021

Conversation

andrii-bodnar
Copy link
Contributor

@andrii-bodnar andrii-bodnar commented Apr 25, 2021 •
edited
Loading

Q A
Branch? 5.x
Bug fix? no
New feature? no
Deprecations? no
Tickets
License MIT
Doc PR

To follow up on #38475, this PR adds Crowdin Provider.
This provider was removed a few weeks ago from the Translation Providers feature by @welcoMattic.

We discussed all the recent changes made on ProviderInterface, TranslatorBagInterface, and others and I already applied these changes to Crowdin Provider.

Also, this Provider is adapted to work with both Crowdin and Crowdin Enterprise.

The todo list to make it ready is:

  • Write integration tests by mocking HTTP Responses

I will make it done before the beginning of May.

@carsonbot
Copy link

Hey!

I see that this is your first PR. That is great! Welcome!

Symfony has a contribution guide which I suggest you to read.

In short:

  • Always add tests
  • Keep backward compatibility (see https://symfony.com/bc).
  • Bug fixes must be submitted against the lowest maintained branch where they apply (see https://symfony.com/releases)
  • Features and deprecations must be submitted against the 5.x branch.

Review the GitHub status checks of your pull request and try to solve the reported issues. If some tests are failing, try to see if they are failing because of this change.

When two Symfony core team members approve this change, it will be merged and you will become an official Symfony contributor!
If this PR is merged in a lower version branch, it will be merged up to all maintained branches within a few days.

I am going to sit back now and wait for the reviews.

Cheers!

Carsonbot

OskarStark
OskarStark reviewed Apr 26, 2021
View reviewed changes
Copy link
Contributor

@OskarStark OskarStark left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for working on this 👍

@andrii-bodnar
Copy link
Contributor Author

@OskarStark thanks for the review 👍

@andrii-bodnar andrii-bodnar changed the title [Translation] Added Crowdin Provider [Translation] Added Crowdin Translation Provider Apr 26, 2021
@andrii-bodnar
Copy link
Contributor Author

It is ready for me for a review 🙂
cc @welcoMattic @OskarStark @Nyholm @nicolas-grekas

stof
stof reviewed Apr 28, 2021
View reviewed changes
@Nyholm Nyholm added this to the 5.3 milestone Apr 28, 2021
@andrii-bodnar
Copy link
Contributor Author

@stof thanks for the review 👍

@andrii-bodnar
Copy link
Contributor Author

@OskarStark @stof, all comments have been addressed except the one related to the concurrency features of symfony/http-client. I need to investigate this issue in more detail 🙂

@welcoMattic welcoMattic mentioned this pull request May 5, 2021
Merged
@andrii-bodnar
Copy link
Contributor Author

@stof @welcoMattic I've addressed all the comments. It looks ready for another review.

welcoMattic
welcoMattic reviewed May 6, 2021
View reviewed changes
Copy link
Member

@welcoMattic welcoMattic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some important comments about loops not being necessary, but this is near to look good to me :+1 :

@andrii-bodnar
Copy link
Contributor Author

@welcoMattic I've addressed all your recent comments.

Replied and not resolved two of them - please take a look and resolve if ok 🙂

P.S. Failed appveyor build seems unrelated to my changes.

@fabpot
Copy link
Member

fabpot commented May 9, 2021

Thank you @andrii-bodnar.

@fabpot fabpot mentioned this pull request May 9, 2021
Closed
@fabpot fabpot merged commit 6bfb509 into symfony:5.x May 9, 2021
@fabpot fabpot mentioned this pull request May 9, 2021
Merged
javiereguiluz added a commit to symfony/symfony-docs that referenced this pull request May 19, 2021
@javiereguiluz
minor #15310 [Translation] Introduce Translation Providers (welcoMattic)
fb1fa0d 
This PR was submitted for the 5.4 branch but it was squashed and merged into the 5.3 branch instead.

Discussion
----------

[Translation] Introduce Translation Providers

Docs for symfony/symfony#38475, symfony/symfony#40926, symfony/symfony#40927, and symfony/symfony#40947

Ready for first review, but I'm not sure that I've written documentation in the right and all required places.

ATM, Translation Providers Bridges packages doesn't exists, so Flex recipes are not created yet.

Commits
-------

943a63f [Translation] Introduce Translation Providers
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@stof stof stof left review comments

@welcoMattic welcoMattic welcoMattic left review comments

@OskarStark OskarStark OskarStark left review comments

@fabpot fabpot fabpot approved these changes

@nicolas-grekas nicolas-grekas nicolas-grekas approved these changes

Assignees
No one assigned
Labels
Ready Status: Reviewed Translation
Projects
None yet
Milestone
5.3
Development

Successfully merging this pull request may close these issues.
8 participants
@andrii-bodnar @carsonbot @fabpot @nicolas-grekas @stof @welcoMattic @OskarStark @Nyholm