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’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

added string concatenation and template string support for javascript #67

Merged
merged 3 commits into from
Dec 6, 2019
Merged

added string concatenation and template string support for javascript #67

merged 3 commits into from
Dec 6, 2019

Conversation

Ouradze
Copy link
Contributor

@Ouradze Ouradze commented Sep 18, 2019 •
edited
Loading

This pull request has two parts:

  • String concatenation like following should be possible after this pull request:
export default {
    name: "greetings",
    computed: {
        greeting_message() {
            return this.$gettext(
              "Hello there!"
              + " I am a concatenated string,"
              + "\\n"
              + " please translate me."
            )
        },
    },
}
  • Use of template string for multiline string without the use of variable interpolation:
export default {
    name: "greetings",
    computed: {
        greeting_message() {
            return this.$gettext(`
Hello there!
I am a multiline string,
please translate me.`)
        },
    }
}

@Ouradze Ouradze requested review from vperron and martigan September 18, 2019 14:45
@codecov-io
Copy link

codecov-io commented Sep 18, 2019 •
edited
Loading

Codecov Report

Merging #67 into master will increase coverage by 0.63%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master     #67      +/-   ##
=========================================
+ Coverage   98.27%   98.9%   +0.63%     
=========================================
  Files           5       5              
  Lines         348     366      +18     
  Branches       60      63       +3     
=========================================
+ Hits          342     362      +20     
+ Misses          6       4       -2
Impacted Files Coverage Δ
src/javascript-extract.js 100% <100%> (+5.88%) ⬆️
src/test-fixtures.js 100% <100%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b593c8c...7d80874. Read the comment docs.

@Ouradze Ouradze changed the title added string concatenation support for javascript added string concatenation and template string support for javascript Sep 18, 2019
@vperron
Copy link
Contributor

vperron commented Oct 15, 2019

@Eclar may I consider this PR ready to review ? :)

@vperron
Copy link
Contributor

vperron commented Oct 21, 2019

@Eclar ?

@Ouradze
Copy link
Contributor Author

Ouradze commented Nov 3, 2019

Oui ! Ready for review. I think :)

vperron
vperron reviewed Nov 5, 2019
View reviewed changes
Copy link
Contributor

@vperron vperron left a comment

Choose a reason for hiding this comment

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

A few notes, and I'd be very pleased with a short changelog for the next version anouncement ! Thanks for the contribution :)

src/javascript-extract.js Outdated
let valueToTranslate = currentToken.value;

if (nextToken.value === '+') {
valueToTranslate = extractConcat(valueToTranslate, allTokens, i + 2 * (argIndex + 1));
Copy link
Contributor

Choose a reason for hiding this comment

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

I think the value i + 2 * (argIndex + 1) could be clearer and more DRY with a variable, like nextTokenIndex

Copy link
Contributor

Choose a reason for hiding this comment

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

which would give code like:

const nextToken = allTokens[nextTokenIndex + 1];
const closingToken = allTokens[nextTokenIndex + 2];
valueToTranslate = extractConcat(valueToTranslate, allTokens, nextToken)

src/javascript-extract.js Outdated
@@ -22,6 +22,17 @@ function toPoItem(withLineNumbers = false) {
return poItem;
}

function extractConcat(value, allTokens, index) {
Copy link
Contributor

Choose a reason for hiding this comment

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

maybe a small comment to explain that this function is intended to concatenate multiline strings would be nice !

src/test-fixtures.js
"Hello there!" +
" I am a concatenated string," +
"\\n" +
" please translate me."
Copy link
Contributor

Choose a reason for hiding this comment

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

I would be interested in a test that shows the concatenation of two strings WITHOUT a line return in between. As such I'm not 100% convinced it is supported.

@vperron
Copy link
Contributor

vperron commented Nov 14, 2019

@Ouradze are you up ? :)

@Ouradze
Copy link
Contributor Author

Ouradze commented Nov 15, 2019

Yes ! On it, hopefully, I'll get that done this weekend :)

@Ouradze
Copy link
Contributor Author

Ouradze commented Dec 5, 2019 •
edited
Loading

@vperron better late than never. I took your comments into account :)

@Ouradze Ouradze requested a review from vperron December 5, 2019 19:55
@Ouradze Ouradze self-assigned this Dec 6, 2019
@vperron vperron merged commit 7d80874 into master Dec 6, 2019
@vperron vperron deleted the eclar/tests branch December 6, 2019 15:45
@vperron
Copy link
Contributor

vperron commented Dec 6, 2019

Thank you so much for your contribution :)

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

@martigan martigan Awaiting requested review from martigan

@vperron vperron Awaiting requested review from vperron

Assignees

@Ouradze Ouradze

Labels
enhancement
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@Ouradze @codecov-io @vperron