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

feat(compiler): add location note to extracted xliff2 files #16791

Merged
merged 1 commit into from May 22, 2017

Conversation

Projects
None yet
7 participants
@ocombe
Contributor

ocombe commented May 15, 2017

What kind of change does this PR introduce?

[x] Bugfix

What is the current behavior?
When we added the xliff2 serializer, we forgot to add the source location at the same time because both PR were merged at about the same time.

See #16531

What is the new behavior?
This PR adds a note with the location of source files to the xliff2 units of extracted files.

Does this PR introduce a breaking change?

[x] No

@ocombe ocombe requested a review from vicb May 15, 2017

@googlebot googlebot added the cla: yes label May 15, 2017

@ocombe ocombe referenced this pull request May 15, 2017

Open

[i18n] plans #16477

0 of 20 tasks complete
@@ -88,17 +88,25 @@ const EXPECTED_XLIFF2 = `<?xml version="1.0" encoding="UTF-8" ?>
<notes>
<note category="description">desc</note>
<note category="meaning">meaning</note>
<note category="location">src/basic.ts:1</note>

This comment has been minimized.

@vicb

vicb May 15, 2017

Contributor

could you add a link to the spec in the commit message

@vicb

vicb May 15, 2017

Contributor

could you add a link to the spec in the commit message

This comment has been minimized.

@ocombe

ocombe May 17, 2017

Contributor

added

@ocombe

ocombe May 17, 2017

Contributor

added

@vicb

This comment has been minimized.

Show comment
Hide comment
@vicb

vicb May 15, 2017

Contributor

LGTM if this conforms to the spec, please add a link in the commit message

Contributor

vicb commented May 15, 2017

LGTM if this conforms to the spec, please add a link in the commit message

@mary-poppins

This comment has been minimized.

Show comment
Hide comment
@mary-poppins

mary-poppins May 17, 2017

The angular.io preview for 4c29acb is available here.

mary-poppins commented May 17, 2017

The angular.io preview for 4c29acb is available here.

@mary-poppins

This comment has been minimized.

Show comment
Hide comment
@mary-poppins

mary-poppins May 17, 2017

The angular.io preview for b49d480 is available here.

mary-poppins commented May 17, 2017

The angular.io preview for b49d480 is available here.

feat(compiler): add location note to extracted xliff2 files
Add source location as a note tag as `<note category="location">path/to/file.ts:start_line[,end_line]</note>`.
`[,end_line]` part is optional and specified only if the end line is different from the start line.

Fixes  #16531
@mary-poppins

This comment has been minimized.

Show comment
Hide comment
@mary-poppins

mary-poppins May 18, 2017

The angular.io preview for 978028b is available here.

mary-poppins commented May 18, 2017

The angular.io preview for 978028b is available here.

@vicb

vicb approved these changes May 18, 2017

@ocombe ocombe changed the title from fix(compiler): add location note to extracted xliff2 files to feat(compiler): add location note to extracted xliff2 files May 18, 2017

@tbosch

tbosch approved these changes May 22, 2017

@chuckjaz chuckjaz merged commit 08dfe91 into angular:master May 22, 2017

4 checks passed

ci/circleci Your tests passed on CircleCI!
Details
cla/google All necessary CLAs are signed
code-review/pullapprove Approved by all reviewer groups.
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
if (message.description || message.meaning) {
const notes = new xml.Tag('notes');

This comment has been minimized.

@panuruj

panuruj May 22, 2017

Contributor

@ocombe According to the XLIFF 2.0 spec, <notes> must have one or more <note> element. So, an empty <notes> is not valid according to the schema.
http://docs.oasis-open.org/xliff/xliff-core/v2.0/os/xliff-core-v2.0-os.html#notes

You can use this schema validator for testing.
http://okapi-lynx.appspot.com/validation

@panuruj

panuruj May 22, 2017

Contributor

@ocombe According to the XLIFF 2.0 spec, <notes> must have one or more <note> element. So, an empty <notes> is not valid according to the schema.
http://docs.oasis-open.org/xliff/xliff-core/v2.0/os/xliff-core-v2.0-os.html#notes

You can use this schema validator for testing.
http://okapi-lynx.appspot.com/validation

This comment has been minimized.

@ocombe

ocombe May 22, 2017

Contributor

it will never be empty since it will contain the url of the source file

@ocombe

ocombe May 22, 2017

Contributor

it will never be empty since it will contain the url of the source file

This comment has been minimized.

@panuruj

panuruj May 22, 2017

Contributor

Hmm... I guess the location will never be empty. This would be okay...

@panuruj

panuruj May 22, 2017

Contributor

Hmm... I guess the location will never be empty. This would be okay...

@ocombe ocombe deleted the ocombe:feature-xliff2-source-16531 branch Jun 8, 2017

asnowwolf added a commit to asnowwolf/angular that referenced this pull request Aug 11, 2017

feat(compiler): add location note to extracted xliff2 files (#16791)
Add source location as a note tag as `<note category="location">path/to/file.ts:start_line[,end_line]</note>`.
`[,end_line]` part is optional and specified only if the end line is different from the start line.

Fixes  #16531

juleskremer added a commit to juleskremer/angular that referenced this pull request Aug 28, 2017

feat(compiler): add location note to extracted xliff2 files (#16791)
Add source location as a note tag as `<note category="location">path/to/file.ts:start_line[,end_line]</note>`.
`[,end_line]` part is optional and specified only if the end line is different from the start line.

Fixes  #16531
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment