Skip to content

Conversation

MaartenX
Copy link

@MaartenX MaartenX commented Nov 21, 2019

Purpose / Goal

This PR adds an option that preserves the order of tags as specified in #197. It is not meant as a finished update (although it works in the tests), but to continue the discussion that was started by @amitguptagwl.

I look forward to seeing what you think of the change.

Type

Please mention the type of PR

  • [ ]Bug Fix
  • [ ]Refactoring / Technology upgrade
  • [x]New Feature

Note : Please ensure that you've read contribution guidelines before raising this PR.

Bookmark this repository for further updates.

@coveralls
Copy link

coveralls commented Nov 21, 2019

Coverage Status

Coverage increased (+0.6%) to 97.006% when pulling 8083bfb on MaartenX:feature/197 into be81aac on NaturalIntelligence:master.

@MaartenX MaartenX force-pushed the feature/197 branch 7 times, most recently from 08da3ab to 84c4336 Compare November 22, 2019 21:29
@amitguptagwl
Copy link
Member

@MaartenX thanks for your effort. I'll be waiting for you to finish this PR. Please be free for any discussion.

@MaartenX MaartenX force-pushed the feature/197 branch 3 times, most recently from 35fc023 to 3597107 Compare November 23, 2019 17:26
@MaartenX
Copy link
Author

@amitguptagwl I fixed the open issues.

I had to find a way to specify in the json that an array of nodes 'has a specific order'. The way I do that currently is by creating an empty key, and the value will be treated as an array that contains the objects in the order they will appear in the xml. I thought about using a 'magic' value key, but since an empty key was not used, I picked that.

@MaartenX MaartenX force-pushed the feature/197 branch 4 times, most recently from 04ab361 to de3729d Compare November 24, 2019 08:51
@amitguptagwl
Copy link
Member

Sorry for the delay. I'll check your PR on next weekend.

@MaartenX
Copy link
Author

MaartenX commented Dec 3, 2019

Any update?

@amitguptagwl
Copy link
Member

Sorry, @MaartenX I was in the impression that the changes are still going on. So hold the review. Will update you.

Copy link
Member

@amitguptagwl amitguptagwl left a comment

Choose a reason for hiding this comment

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

There is a small suggestion. I haven't checked for the complete PR yet. I'll probably checkout and understand the flow for better undersanding and output.

@amitguptagwl
Copy link
Member

I have noticed that the possible values of preserveOrder are text and true in your code change. But I couldn't find value text has been used for any different implementation. Can you please explain to me about that?

As you mentioned that you have skipped CDATA in this PR, I wanted to understand if it is because of reducing the complexity of the PR, or it was not feasible at all. Because I just wanted to ensure that the current approach can be extended to adopt future changes.

@amitguptagwl
Copy link
Member

@MaartenX any update?

@MaartenX MaartenX force-pushed the feature/197 branch 2 times, most recently from 9fe2f18 to 92ca31f Compare January 3, 2020 14:20
@schoste
Copy link

schoste commented Mar 31, 2020

How is the status of the PR?

@qhkm
Copy link

qhkm commented Sep 3, 2020

Been waiting for this PR too

@lwestfall
Copy link
Contributor

Me too, coming up on a year since PR was started. Any way we can get priority on this? Would a donation help? I can't use this until this is merged.

@MaartenX
Copy link
Author

MaartenX commented Sep 3, 2020

Hey guys, this changes was done because of a time-critical update I needed, so I used it from my fork. I'll try and pick it up again since you are interested, but that won't be in the coming week (really busy with work).

@lwestfall
Copy link
Contributor

Appreciate the update!

@amitguptagwl
Copy link
Member

amitguptagwl commented Sep 6, 2020

@MaartenX @lwestfall if you check v4 branch. I've already handled this feature. However, I have not completed the tests so I can't publish the changes. I'm currently busy in another opensource project that I'm hoping to complete in few weeks.

Any help to migrate the tests from master branch to v4 branch will speed the things up. The only thing will be left is to update the JSON2XML parser for new changes.

So I'm closing this PR.

@samuelcolvin
Copy link

I really need this feature.

Is there anywhere we can track the progress of v4?

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.

7 participants