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.

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

Add goup handling in JSON transformations. #50

Merged
merged 9 commits into from
Jan 2, 2022

Conversation

marmarosi
Copy link
Contributor

Resolves #49. Usage:

const res = targetOfjs(js, {includeGroups: true})
const res = sourceOfjs(js, {includeGroups: true})

@adrai
Copy link
Contributor

adrai commented Jan 1, 2022

Can you include a test?

@adrai
Copy link
Contributor

adrai commented Jan 1, 2022

Do you also want to support groups in the createxliff functions? If so, should we skip the includeGroups option completely and respect groups always (as soon as the object property is an object and not a string?)
What do you think?

@marmarosi
Copy link
Contributor Author

It seems feasible. I give it a try...

@adrai
Copy link
Contributor

adrai commented Jan 2, 2022

Can you write the tests like the existing ones?

@marmarosi
Copy link
Contributor Author

marmarosi commented Jan 2, 2022

  1. sourceOfjs & targetOfjs:
    options { includeGroups: true } removed
    handle nested groups as well
    handle group notes

2, js2xliff & js2xliff12:
handle groups and nested groups
handle group notes

  1. Bugfix: js2xliff & js2xliff12 convert multiple notes correctly
    instead of <note>note1,note2</note>
    now <note>note1</note><note>note2</note>

I cannot run browserify and mocha tests on Windows, therefore created some functional test in test/mj/*.

@marmarosi
Copy link
Contributor Author

I get the following errors:

$ ./node_modules/.bin/browserify --standalone xliff cjs/index.js -o xliff.js

D:\Development\logikum\xliff\cjs\index.js:1
import xliff2js from './xliff2js.js';
^
ParseError: 'import' and 'export' may appear only with 'sourceType: module'

$ ./node_modules/.bin/mocha test -R spec

D:\Development\logikum\xliff\cjs\index.js:1
import xliff2js from './xliff2js.js';
^^^^^^

SyntaxError: Cannot use import statement outside a module

@adrai
Copy link
Contributor

adrai commented Jan 2, 2022

Tests should run also on windows... what's the problem?

@adrai
Copy link
Contributor

adrai commented Jan 2, 2022

Which node version are you using?

@marmarosi
Copy link
Contributor Author

node.js v16.3

@marmarosi
Copy link
Contributor Author

@adrai
Copy link
Contributor

adrai commented Jan 2, 2022

Ok, it should work on windows now: https://github.com/locize/xliff/runs/4684040716?check_suite_focus=true
Can you update your branch?

@marmarosi
Copy link
Contributor Author

I check it...

@marmarosi
Copy link
Contributor Author

Funny... I see your changes on Github, but git does not fetch anything.

lib/createjs.js Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
@adrai
Copy link
Contributor

adrai commented Jan 2, 2022

@marmarosi
Copy link
Contributor Author

Still cannot run tests. I did successfully:
git fetch upstream
git checkout master
git merge upstream/master
...and I get the same errors. Even when I change to include-groups branch.

@adrai
Copy link
Contributor

adrai commented Jan 2, 2022

From what I see published here (https://github.com/marmarosi/xliff/tree/include-groups), I can't see any updates?

And based on the Github Windows tests, it works successfully: https://github.com/locize/xliff/runs/4684040716?check_suite_focus=true

@adrai
Copy link
Contributor

adrai commented Jan 2, 2022

If you prefer, you can delete everything and create a new PR...

@marmarosi
Copy link
Contributor Author

Adding ,/node_modules/.bin to PATH resolved my babel problem. However, some tests now fails, I suppose as a result of my amendments. I need to review them...

@marmarosi
Copy link
Contributor Author

I think the PR is error free now.

@adrai adrai merged commit 2dc2ff3 into locize:master Jan 2, 2022
@adrai
Copy link
Contributor

adrai commented Jan 2, 2022

thank you for your contribution
released with v6.0.0

@marmarosi marmarosi deleted the include-groups branch March 6, 2022 10:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

targetOfjs does not convert groups
2 participants