Check new translations strings in PR #34
Conversation
boherm
commented
Apr 9, 2024
•
boherm
commented
| Questions | Answers |
|---|---|
| Description? | Add command to check new translations strings in PR. Migration from Prestonbot. |
| Type? | improvement |
| BC breaks? | no |
| Deprecations? | no |
| Fixed ticket? | Fixes PrestaShop/PrestaShop#34271 |
| Sponsor company | PrestaShop SA |
| How to test? |
boherm
force-pushed
the
#34271-check-new-translations
branch
from
ae76bee
to
a1cdefe
Compare
There was a problem hiding this comment.
Thanks @boherm
it's nice the code is very clear with each command having its responsibility, great job!
I just added some comments to improve the tests, and some suggestions to improve the feature itself but I'm not sure about the initial scope so maybe they are not relevant.
Let me know hat you think about them.
| $this->readyForReviewColumnName, | ||
| ), | ||
| ]; | ||
|
|
||
| if ('PrestaShop' === $repoOwner && 'PrestaShop' === $repoName) { |
There was a problem hiding this comment.
Ok for now, but do you think it could work on other repositories as well? (mostly for modules) Or would it require some improvements in another PR?
There was a problem hiding this comment.
In fact, it should work with others repositories as well, and I would like to remove this condition to test if it's works!
| $newTranslationsKeys = []; | ||
| $newDomains = []; | ||
| foreach ($translations as $domain => $keys) { | ||
| $catalog = $this->catalogProvider->getTranslationsCatalog('en-US', $domain); |
There was a problem hiding this comment.
So for no the catalog check is always the v9 right?
Maybe it should be more dynamic to adapt to the target branch? So that any patch PR on 8.1.x would indicate changes based on the appropriate catalog.
This could be improved in another PR though
There was a problem hiding this comment.
Prestonbot seems to work always with the v9, so I pasted this behaviour here, but yeah, you're totally right!
I will change that!
There was a problem hiding this comment.
Ah so this would be a nice improvement actually
| $this->checkTranslationsCommandHandler = new CheckTranslationsCommandHandler($this->prRepository, $this->catalogProvider); | ||
|
|
||
| $this->prRepository->method('find')->willReturn($this->pr); | ||
| $this->prRepository->method('getDiff')->willReturn(PullRequestDiff::parseDiff($this->prId, (string) $prDiffContent)); |
There was a problem hiding this comment.
You should mock addTranslationsComment as well, probably not the whole content but you could add some assertions on the expected parameters, this would allow validating the logic of telling new wordings and new domains apart
You can mock it in each test case and use a @dataProvider on a single test methods, this could look like this:
/**
* @dataProvider getWordingsData
*/
public function testNewWordingsDetection(array $wordingsInCatalog, array $expectedNewDomains, array $expectedNewWordings): voidYou can then pass the parameters to the dynamick mocks using the willReturn and with mock methods
There was a problem hiding this comment.
This would allow you to check more detailed use cases:
- all wordings are new
- no wordings are ne
- only some of the wordings are ne
If you're motivated you could even vary the diff in input, although it's harder to create those fixtures
boherm
force-pushed
the
#34271-check-new-translations
branch
from
a1cdefe
to
071a561
Compare
| { | ||
| $PSVersion = 9; | ||
|
|
||
| if ('PrestaShop' === $pullRequest->getId()->repositoryName && str_starts_with($pullRequest->getTargetBranch(), '8')) { |
There was a problem hiding this comment.
Will this code works with PR targeting branch 1.7.8.x ?
There was a problem hiding this comment.
I don't really think that theses 1.7.x branches are necessary. Maybe I'm wrong?
There was a problem hiding this comment.
I agree that kanbanbot does not need to verify 1.7.x branches 👍 we will not add new wordings in 1.7.x versions
But I'm worried kanbanbot would spam PRs for 1.7.x with invalid messages.
But it's a minot usecase 😄 if it happens we can always fix it later
Really tempting !
|
