8000 [Translation] Allow using dashes in locale when linting Xliff files by localheinz · Pull Request #40172 · symfony/symfony · GitHub
[go: up one dir, main page]
Skip to content

[Translation] Allow using dashes in locale when linting Xliff files #40172

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
Feb 16, 2021
Merged

[Translation] Allow using dashes in locale when linting Xliff files #40172

merged 1 commit into from
Feb 16, 2021

Conversation

localheinz
Copy link
Contributor
@localheinz localheinz commented Feb 12, 2021 •
edited
Loading
Q A
Branch? 4.4
Bug fix? yes
New feature? no
Deprecations? no
Tickets fixes #40170
License MIT
Doc PR n/a

This pull request

  • asserts that the XliffLintCommand succeeds linting an Xliff file where both the the target language and the locale in the file name use dashes as separators
  • adjusts the XliffLintCommand to allow using the same value for target language and locale in the corresponding file name

@carsonbot
Copy link

Hey!

I see that more good work is coming your way.

To help keep things organized, we don't allow "Draft" pull requests. Could you please click the "ready for review" button or close this PR and open a new one when you are done?

Note that a pull request does not have to be "perfect" or "ready for merge" when you first open it. We just want it to be ready for a first review.

Cheers!

Carsonbot

@localheinz localheinz marked this pull request as ready for review February 12, 2021 19:12
@carsonbot carsonbot added this to the 4.4 milestone Feb 12, 2021
@carsonbot carsonbot changed the title [Translation] Allow using dashes in locale when linting Xliff files [Translator] Allow using dashes in locale when linting Xliff files Feb 12, 2021
@localheinz localheinz force-pushed the fix/dash branch 2 times, most recently from 147627a to 4078cdb Compare February 12, 2021 19:16
OskarStark
OskarStark approved these changes Feb 12, 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.

Thanks, I am just not sure if this should be considered a bugfix or a new feature 🧐

@localheinz
Copy link
Contributor Author
localheinz commented Feb 12, 2021 •
edited
Loading

@OskarStark

A bug fix would be great, since the corresponding application can not yet upgrade to symfony/translation:^5.

🤓

@OskarStark
Copy link
Contributor

I know, from my perspective it can be considered a bugfix, as you explained it is conform with the spec 👍🏻

@localheinz localheinz changed the title [Translator] Allow using dashes in locale when linting Xliff files [Translation] Allow using dashes in locale when linting Xliff files Feb 12, 2021
@localheinz localheinz changed the title [Translation] Allow using dashes in locale when linting Xliff files [Translator] Allow using dashes in locale when linting Xliff files Feb 14, 2021
@localheinz localheinz force-pushed the fix/dash branch 2 times, most recently from b3c8c14 to 7bfb693 Compare February 15, 2021 09:06
@localheinz localheinz changed the title [Translator] Allow using dashes in locale when linting Xliff files [Translation] Allow using dashes in locale when linting Xliff files Feb 15, 2021
@fabpot
Copy link
Member
fabpot commented Feb 16, 2021

Thank you @localheinz.

@fabpot fabpot merged commit aa21944 into symfony:4.4 Feb 16, 2021
@localheinz localheinz deleted the fix/dash branch February 16, 2021 07:18
@localheinz
Copy link
Contributor Author

Thank you, @derrabus, @fabpot, @nicolas-grekas, and @OskarStark!

@sschueller
Copy link

Will this change validate de-x-app123 or de-x-app123-a as valid? According to xliff 1.2 (urn:oasis:names:tc:xliff:document:1.2) this should be validated with xsd:language which is [a-zA-Z]{1,8}(-[a-zA-Z0-9]{1,8})*

@OskarStark
Copy link
Contributor

Open for a PR including a testcase so we can talk about code and an example?

This was referenced Mar 4, 2021
Merged
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@fabpot fabpot fabpot approved these changes

@OskarStark OskarStark OskarStark approved these changes

@derrabus derrabus derrabus approved these changes

@nicolas-grekas nicolas-grekas Awaiting requested review from nicolas-grekas

Assignees
No one assigned
Labels
Bug Status: Reviewed Translation
Projects
None yet
Milestone
4.4
Development

Successfully merging this pull request may close these issues.
7 participants
@localheinz @carsonbot @OskarStark @fabpot @sschueller @nicolas-grekas @derrabus
0