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

XMLBuilder: introduce initial indentation level option #566

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

XMLBuilder: introduce initial indentation level option #566

wants to merge 3 commits into from

Conversation

moki
Copy link

@moki moki commented Apr 23, 2023

Purpose / Goal

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 to start generating xml document starting from the specified indentation level.

I explain the reasoning behind introducing this feature extensively in the #565

Closes: #565

Type

  • Bug Fix
  • Refactoring / Technology upgrade
  • New Feature

moki added 3 commits April 23, 2023 21:44
@moki
test(j2x): initial indentation level option
d6ec5e2
@moki
feat(xml-builder): start indentation from specified indentation level
8fca642 
introduces new XMLBuilder option: initialIndentationLevel,
allows to build xml starting from the provided indentaion level.

default value is 0, so that previous default behaviour is unchanged
@moki
chore(docs): XMLBuilder initialIndentationLevel example
0045494
@moki moki changed the title xmlBuilder: introduce initial indentation level option XMLBuilder: introduce initial indentation level option Apr 23, 2023
@moki moki mentioned this pull request Apr 23, 2023
Open
2 tasks
@amitguptagwl
Copy link
Member

These changes seems fine to me. I've to think more on option name, if something else be more concise.

@coveralls
Copy link

coveralls commented Apr 25, 2023 •
edited
Loading

Coverage Status

Coverage: 98.204% (+0.01%) from 98.194% when pulling 0045494 on moki:moki-codes/builder-initial-indentation-level-option into b6cad83 on NaturalIntelligence:master.

@amitguptagwl
Copy link
Member

@moki I'm just wondering if we should name it as "margin". It might not be a good suggestion so I'm thinking more like in this way.

@moki
Copy link
Author

moki commented Apr 26, 2023

margin

i guess margin describes well what one will actually see in the end result.

but doesn't fit well regarding terminology...

What do you think about startIndent or maybe indentPrefix?

@amitguptagwl
Copy link
Member

indentPrefix looks good but may confuse users that it might be used everytime. startIndent in itself is not explanatory. Let me think more like marginLevel or something else.

@moki
Copy link
Author

moki commented May 2, 2023

@amitguptagwl sorry for pinging you, but do you have any updates on the option naming?

@amitguptagwl
Copy link
Member

@moki Sorry I was on a long vacation and now sick. I'll check it back as soon as I feel little good.

@moki
Copy link
Author

moki commented May 17, 2023

@moki Sorry I was on a long vacation and now sick. I'll check it back as soon as I feel little good.

its all good, get well soon!

@moki
Copy link
Author

moki commented Jun 14, 2023 •
edited
Loading

hey @amitguptagwl, have you had time to revisit this?

@amitguptagwl
Copy link
Member

Hey @moki , I've completed the major development of v5 of this library few weeks back. Hence, I'm avoiding any change in the current version of library until or unless some bug is reported. However, the new version has some performance issue. So I'll merge this PR if v5 is discarded or I'll inform you once it is released. For the new version, you can check my post under discussion.
Thanks for your effort and sorry for the delay.

@moki
Copy link
Author

moki commented Jun 15, 2023

Hey @moki , I've completed the major development of v5 of this library few weeks back. Hence, I'm avoiding any change in the current version of library until or unless some bug is reported. However, the new version has some performance issue. So I'll merge this PR if v5 is discarded or I'll inform you once it is released. For the new version, you can check my post under discussion. Thanks for your effort and sorry for the delay.

alright keep me posted, also do you plan to release the draft of the v5 as a separate branch.

i mean i would like to "rebase" this PR onto it.

in quotes - because prolly i will have to re-write my pr for it.

@amitguptagwl
Copy link
Member

Hi @moki, thought v5 is supposed to support many awaited features, it's speed is downgraded by 4-5 times. I have few ideas of improving it by shifting to old js style. However, not getting time to work due to job shift. As soon as I fix the performance issue in new version, I'll cut the branch and will let you know. Thanks

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Introduce initialIndentationLevel option for the XMLBuilder
3 participants
@moki @amitguptagwl @coveralls