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.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fixes MT for languages with faulty BCP code #2124

Merged
merged 1 commit into from Mar 21, 2023
Merged

Fixes MT for languages with faulty BCP code #2124

merged 1 commit into from Mar 21, 2023

Conversation

JoeyStk
Copy link
Contributor

@JoeyStk JoeyStk commented Mar 14, 2023 •
edited

Short description

This PR fixes an error that occurred while using the MT for English or Portuguese. With this PR now it gets checked if a. the BCP47 code is in the correct format as expected, and either returns an error message or for the edge case that the BCP47 tag for English or Portuguese were incorrect sets en-GB respectively pt-PT as default.

Proposed changes

  • Add check if BCP47 code is correct
  • Set pt-PT and en-GB as default values if the BCP47 code wasn't correct
  • Return an error message for all other cases in which the BCP47 code wasn't correct
  • Remove the check from the main function and put it in a separate function in order to pass pylint (too many if-branches in one function 15 out of 12). However I had to add the check "if target_language_key is None" to the main function, which is quite ugly. If someone else has a better idea for this, please let me know 馃槃

Side effects

  • I didn't into any

Resolved issues

Fixes: #2116

Pull Request Review Guidelines

@JoeyStk JoeyStk requested a review from a team as a code owner March 14, 2023 15:07
@JoeyStk JoeyStk force-pushed the bugfix/fix_exception_with_faulty_BCP_tag branch from 4355e69 to 5a4088f Compare March 14, 2023 15:07
@codeclimate
Copy link

codeclimate bot commented Mar 14, 2023 •
edited

Code Climate has analyzed commit 8ca924b and detected 0 issues on this pull request.

The test coverage on the diff in this pull request is 18.1% (50% is the threshold).

This pull request will bring the total coverage in the repository to 75.8% (0.0% change).

View more on Code Climate.

charludo
charludo requested changes Mar 15, 2023
View reviewed changes
Copy link
Contributor

@charludo charludo left a comment

Choose a reason for hiding this comment

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

Thanks a lot! 馃帀

I only have a couple suggestions, mostly to make get_target_language_key a little more succinct.

integreat_cms/deepl_api/utils.py Outdated Show resolved Hide resolved
integreat_cms/deepl_api/utils.py Outdated Show resolved Hide resolved
integreat_cms/deepl_api/utils.py Outdated Show resolved Hide resolved
timobrembeck
timobrembeck requested changes Mar 15, 2023
View reviewed changes
integreat_cms/deepl_api/utils.py Outdated Show resolved Hide resolved
@JoeyStk JoeyStk force-pushed the bugfix/fix_exception_with_faulty_BCP_tag branch 4 times, most recently from ea5ef3d to 119c7d7 Compare March 16, 2023 12:36
timobrembeck
timobrembeck requested changes Mar 16, 2023
View reviewed changes
integreat_cms/cms/templates/_base.html Outdated Show resolved Hide resolved
integreat_cms/cms/templates/languages/language_form.html Outdated Show resolved Hide resolved
integreat_cms/cms/views/events/event_list_view.py Outdated Show resolved Hide resolved
integreat_cms/deepl_api/utils.py Outdated Show resolved Hide resolved
@JoeyStk JoeyStk force-pushed the bugfix/fix_exception_with_faulty_BCP_tag branch 5 times, most recently from d4841ac to a1e58f9 Compare March 21, 2023 12:13
timobrembeck
timobrembeck requested changes Mar 21, 2023
View reviewed changes
integreat_cms/cms/templates/languages/language_form.html Outdated Show resolved Hide resolved
integreat_cms/deepl_api/utils.py Outdated Show resolved Hide resolved
integreat_cms/deepl_api/utils.py Outdated Show resolved Hide resolved
integreat_cms/locale/de/LC_MESSAGES/django.po Outdated Show resolved Hide resolved
integreat_cms/locale/de/LC_MESSAGES/django.po Outdated Show resolved Hide resolved
integreat_cms/locale/de/LC_MESSAGES/django.po Outdated Show resolved Hide resolved
integreat_cms/deepl_api/utils.py Outdated Show resolved Hide resolved
@JoeyStk JoeyStk force-pushed the bugfix/fix_exception_with_faulty_BCP_tag branch from 6b54ffb to ed499d2 Compare March 21, 2023 15:47
@JoeyStk JoeyStk force-pushed the bugfix/fix_exception_with_faulty_BCP_tag branch from ed499d2 to 8ca924b Compare March 21, 2023 15:48
@JoeyStk
Copy link
Contributor Author

JoeyStk commented Mar 21, 2023

Thank you a lot for your feedback. The changes are applied now. Feel free to review again :)

timobrembeck
timobrembeck approved these changes Mar 21, 2023
View reviewed changes
Copy link
Member

@timobrembeck timobrembeck left a comment

Choose a reason for hiding this comment

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

馃帀

@JoeyStk JoeyStk merged commit a337301 into develop Mar 21, 2023
5 checks passed
@JoeyStk JoeyStk deleted the bugfix/fix_exception_with_faulty_BCP_tag branch March 21, 2023 20:16
This was referenced Mar 22, 2023
Closed
Merged
This was referenced Mar 22, 2023
Open
Merged
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@timobrembeck timobrembeck timobrembeck approved these changes

@charludo charludo charludo approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

DeepLException during MT for languages with faulty bcp tag
3 participants
@JoeyStk @timobrembeck @charludo