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

Introduce initialIndentationLevel option for the XMLBuilder #565

Open
1 of 2 tasks
moki opened this issue Apr 23, 2023 · 6 comments · May be fixed by #566
Open
1 of 2 tasks

Introduce initialIndentationLevel option for the XMLBuilder #565

moki opened this issue Apr 23, 2023 · 6 comments · May be fixed by #566

Comments

@moki
Copy link

moki commented Apr 23, 2023

Hello there! I’ve started using XMLParser/XMLBuilder in my project markdown-translation and arrived at use case that is not really supported in the XMLBuilder.

First i render XML template/wrapper which makes skeleton base of the rendered document with {format: true} options.

example of the generated template:

<?xml version="1.0" encoding="UTF-8"?>
<xliff xmlns="urn:oasis:names:tc:xliff:document:1.2" version="1.2">
  <file original="file.md" source-language="ru-RU" target-language="en-US" datatype="markdown">
    <header>
      <skeleton>
        <external-file href="file.skl.md"></external-file>
      </skeleton>
    </header>
    <body>
    <!-- split point -->
    </body>
  </file>
</xliff>

After rendering template i split it at the <!-- split point --> so that later i can dynamically render <trans-unit></trans-unit> with content inside the body tag.

The problem is that later when i render each trans-unit tag XMLBuilder unaware of the fact that trans-unit tag is going to be injected into other document(template) at the level: 3 point of indentation.

Result is going to be something like this:

<?xml version="1.0" encoding="UTF-8"?>
<xliff xmlns="urn:oasis:names:tc:xliff:document:1.2" version="1.2">
  <file original="file.md" source-language="ru-RU" target-language="en-US" datatype="markdown">
    <header>
      <skeleton>
        <external-file href="file.skl.md"></external-file>
      </skeleton>
    </header>
    <body>
<trans-unit>
  <source>str</source>
  <target>target</target>
</trans-unit>
    </body>
  </file>
</xliff>

Hence i propose the solution - Introduce new option initialIndentationLevel and initialize builder with it instead of the literal value 0.

The option initialIndentationLevel is going to default to 0 so that default behaviour will be preserved.

Yet will allow my use case and generally make it more convenient building part of the XML document in the pretty format.

Please let me know what you think about it.

There is already PR for this feature: #566.

Would you like to work on this issue?

  • Yes
  • No
@github-actions
Copy link

We're glad you find this project helpful. We'll try to address this issue ASAP. You can vist https://solothought.com to know recent features. Don't forget to star this repo.

@moki moki linked a pull request Apr 23, 2023 that will close this issue
3 tasks
@amitguptagwl
Copy link
Member

What if you set indentBy to \t\t\t\t, depends on how much indentation you need?

@moki
Copy link
Author

moki commented Apr 24, 2023

What if you set indentBy to \t\t\t\t, depends on how much indentation you need?

as far as i can tell from examining the code base that option will just overwrite indentation symbols.

i.e. if we set indentBy to ' ' it will make every nested indentation ' '

<trans-unit>
...<source>str</source>
...<target>target</target>
</trans-unit>

making it unfittable for my usecase(trans-unit not indented, even tho nested indentation can be set)

tbh at first i thought this option will help me solve my problem, but unfortunately it doesn't.

also for now i have an mvp version with only trans-units, later it is going to be group of multiple trans-units with XLF specific markup inside, making nesting more deep.

<group>
..<trans-unit id="1">
....<source>
......<g>[</g>str
....</source>
....<target>target</target>
..</trans-unit>
</group>

etc...

@amitguptagwl
Copy link
Member

First of all, what is the need of this? formatting doesn't cause parsing issue. And I believe, you don't have to show it. What you can do probably is format the whole document through prettify at last.

@moki
Copy link
Author

moki commented Apr 24, 2023

formatting doesn't cause parsing issue.

I don't remember ever stating this.

What you can do probably is format the whole document through prettify at last.

not sure if i get what you are trying to say, but if you are saying that i can still use XMLBuilder without this feature, Then yes i can and i do. but it is far from optimal.

i am rendering like this:

<trans-unit>
..<source>str</source>
..<target>target</target>
</trans-unit>

and then split on each like and prefixing with appropriate indentation to make it look like this

......<trans-unit>
........<source>str</source>
........<target>target</target>
......</trans-unit>

also i could generate javascript plain object dynamically and only then use XMLBuilder in the end. But that will make project hard dependent on your project and that's not the architecture solution i am up to.

The bottom line this change would make my usage of the library convenient and optimal(runtime complexity wise).

So let me know if you are up to merging this(introducing this functional into the library).

If there is any other work that needs to be done to make it land into master, let me know.

Also i do understand that my usecase sounds esoteric and i would understand if you wouldn't want to merge it. But since it is an easy change that doesn't break things and just allows to start indenting from different level instead of constant 0 literal i decided to try to land it.

@amitguptagwl
Copy link
Member

Let me check your implementation. However, initialIndentationLevel doesn't seem a good name or generic name to me. So I'l have to think about it.

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 a pull request may close this issue.

2 participants