feat(markdown-docx): add optional transformer - #397 by K-Kumar-01 · Pull Request #441 · accordproject/markdown-transform

K-Kumar-01 · 2021-08-10T12:50:40Z

Signed-off-by: k-kumar-01 kushalkumargupta4@gmail.com

Add the logic to transform Optional nodes from CiceroMark<->OOXML.

Changes

transformation logic:OOXML<->CiceroMark
tests for optional
update tests for other(xml changes in variable and clause)

Author Checklist

Ensure you provide a DCO sign-off for your commits using the --signoff option of git commit.
Vital features and changes captured in unit and/or integration tests
Commits messages follow AP format
Extend the documentation, if necessary
Merging to master from fork:branchname

transformation logic:OOXML<->CiceroMark tests for optional update tests for other(xml changes in variable and clause) Signed-off-by: k-kumar-01 <kushalkumargupta4@gmail.com>

dselman

I'm finding the code hard to follow, as you add more special cases. In general terms I would structure the code as a transformation from treeA -> treeB with a switch statement to handle different nodes in treeA, converting them to nodes in treeB. In particular I found the part where you compare lengths of arrays to hard coded values confusing.

dselman · 2021-08-10T13:52:09Z

          </w:rPr>
          <w:alias w:val="Shipper1 | org.accordproject.organization.Organization"/>
-          <w:tag w:val="shipper"/>
+          <w:tag w:val="org.accordproject.ciceromark.Variable----shipper"/>


What are tag and alias used for? Could we make them consistent. E.g.
org.acme.Thing/name ?

w:alias: the name which is displayed by word on hovering on content control.
w:tag: Bind the office JS API handles for elements with similar tags.

Could we make them consistent. E.g.org.acme.Thing/name ?

I do not understand what exactly you mean here. I have introduced the first thing to distinguish the type of content control as we discussed in the meeting.

K-Kumar-01 · 2021-08-10T15:21:35Z

In general terms I would structure the code as a transformation from treeA -> treeB with a switch statement to handle different nodes in treeA, converting them to nodes in treeB.

Actually, this is what I am doing but I was handling it with if statements.

In particular I found the part where you compare lengths of arrays to hard coded values confusing.

I agree here but this is done to insert the sibling children of a node under the same parent.

EDIT:

K-Kumar-01 · 2021-08-10T16:53:12Z

@algomaster99
Requesting your reviews also.

K-Kumar-01 · 2021-08-11T15:41:30Z

@dselman @algomaster99
I have completed the transformation for the formula in another branch. Since the branch is extension of this one, we can merge that once this PR gets merged.

algomaster99

First iteration of review done. Look at the comments.

Also, add a test case when hasSome is false. You will need to see how nodes behave in that case. Currently, you have made nodes = whenSome.

transform both whenSome and whenNone nodes add tests for hasSome:false update the xml Signed-off-by: k-kumar-01 <kushalkumargupta4@gmail.com>

K-Kumar-01 · 2021-08-12T14:20:36Z

@algomaster99
I have incorporated the changes to transform hasSome:false condition in optional.
Let me know if there are any other changes required.

EDIT: The conditions may seem to be a bit terrifying 😅 but it was the only implementation I can think of.

algomaster99

Dropped some comments which I think you really need to look into. Please try to incorporate them. I will try to take out some time and refactor your code a bit over the weekend.

…iables, add generic method for nesting Signed-off-by: k-kumar-01 <kushalkumargupta4@gmail.com>

K-Kumar-01 · 2021-08-14T12:05:36Z

@algomaster99
Made the desired changes.

K-Kumar-01 · 2021-08-17T03:28:39Z

@algomaster99 @dselman
Requesting your reviews.

algomaster99

Left some comments. There are some things I don't understand so I might give more reviews on this PR in future.

algomaster99 · 2021-08-17T06:53:33Z

+        // Contains the tags for optional or conditional nodes.
+        // Optional/conditional themselves can act contain multiple inline nodes.
+        // However, they are not block elements and are present inside
+        // paragraph or clause nodes so an extra array is needed to store their info.


@K-Kumar-01 You forgot to remove this array.

Signed-off-by: k-kumar-01 <kushalkumargupta4@gmail.com>

K-Kumar-01 · 2021-08-17T11:29:48Z

@algomaster99
Made the changes. Let me know if there are any other changes required.

algomaster99 · 2021-08-17T11:45:46Z


 /**
- * Inserts Baskerville Old Face font family OOXML tag.
+ * Inserts Baskerville Old Face font family OOXML tag to distinguish nodes of whenNone/whenFalse


Try to put yourself in a person's shoe who is not that familiar with this code.

They try to interpret this function and they found out that it has something to do with optional or condition nodes. They look at the CiceroMark properties and see that in fact, whenSome and whenNone exist. However, they would definitely question "what are whenTrue and whenFalse for?" Given then context, they would deduce that whenTrue and whenFalse are also attributes in the CiceroMark which is false!

In fact this is true wherever you have metioned the above terms so you need to rectify that. Probably a better wording would be:

Inserts a different fontface so that the hidden contents can be distinguished. There can be two types of hidden content - 1) when `hasSome` is true, the content inside `whenNone` is hidden. 2) when `hasSome` is false, the content inside `whenSome` is hidden.

I know this takes time and it may not be commensurate for the time you spend thinking over it. But I can promise you that you only will forget the same code you have written 4 months later given you don't work on this project for that long.

@algomaster99 Actually, this is not quite the right description. the font family is for else condition always. It does not matter whether they are hidden or visible.

If they are visible, then we do not want to change the font.

@algomaster99
Does this sound right?

Inserts a different font family so that the conditional/optional content(else condition) can be distinguished. They can be of following types:- 1) `whenNone` nodes for optional(to be distinguished from `whenSome`) 2) `whenFalse` nodes for conditional(to be distinguished from `whenTrue`)

Okay, so the font face has been introduced to handle conditional as well? I thought you would make a separate PR for that.

@algomaster99
It will automatically deal with that when I make the PR for conditional. Optional and Conditional are identically same. Just some node keys changing.

@algomaster99
I will update the JSDOC for conditional in that PR. Currently, I will only make it only for optional. Does it sound fine?

Sounds good. You could have applied the same logic for the variables too but that's okay. We can merge this one.

Anyway, just try to focus on one feature at a time. It causes confusion otherwise.

algomaster99 · 2021-08-17T12:16:51Z

@K-Kumar-01 Shall I proceed for merge if all tests are passing?

Signed-off-by: k-kumar-01 <kushalkumargupta4@gmail.com>

K-Kumar-01 · 2021-08-17T12:19:33Z

@algomaster99 Sure
was pushing the updated JSDOC comment for optional for now.
WIll later change it for both conditional and optional as mentioned.

feat(markdown-docx): add optional transformer - #397

9c00a23

transformation logic:OOXML<->CiceroMark tests for optional update tests for other(xml changes in variable and clause) Signed-off-by: k-kumar-01 <kushalkumargupta4@gmail.com>

K-Kumar-01 added this to the Implement optional milestone Aug 10, 2021

K-Kumar-01 requested review from algomaster99 and dselman August 10, 2021 12:50

dselman reviewed Aug 10, 2021

View reviewed changes

K-Kumar-01 requested a review from dselman August 10, 2021 18:21

algomaster99 suggested changes Aug 12, 2021

View reviewed changes

Comment thread packages/markdown-docx/src/ToCiceroMarkVisitor.js Outdated

Comment thread packages/markdown-docx/src/ToOOXMLVisitor/index.js Outdated

feat(markdown-docx): update optional transformer

6f0f606

transform both whenSome and whenNone nodes add tests for hasSome:false update the xml Signed-off-by: k-kumar-01 <kushalkumargupta4@gmail.com>

K-Kumar-01 requested a review from algomaster99 August 12, 2021 14:19

algomaster99 suggested changes Aug 13, 2021

View reviewed changes

refactor(markdown-docx): replace class variables with local scope var…

475e6d8

…iables, add generic method for nesting Signed-off-by: k-kumar-01 <kushalkumargupta4@gmail.com>

K-Kumar-01 requested a review from algomaster99 August 14, 2021 12:05

algomaster99 suggested changes Aug 17, 2021

View reviewed changes

refactor(markdown-docx): variable name, JSDOC, function arguments

4b31573

Signed-off-by: k-kumar-01 <kushalkumargupta4@gmail.com>

algomaster99 reviewed Aug 17, 2021

View reviewed changes

K-Kumar-01 mentioned this pull request Aug 17, 2021

Refactor the transfomer(OOXML<->CiceroMark) #451

Open

4 tasks

refactor(markdown-docx): JSDOC update

aec3672

Signed-off-by: k-kumar-01 <kushalkumargupta4@gmail.com>

algomaster99 merged commit de694a2 into algoo-ooxml Aug 17, 2021

algomaster99 deleted the k-kumar-01/i397/optional-transformer branch August 17, 2021 13:59

Conversation

K-Kumar-01 commented Aug 10, 2021

Changes

Author Checklist

Uh oh!

dselman left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

K-Kumar-01 commented Aug 10, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

K-Kumar-01 commented Aug 10, 2021

Uh oh!

K-Kumar-01 commented Aug 11, 2021

Uh oh!

algomaster99 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

K-Kumar-01 commented Aug 12, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

algomaster99 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

K-Kumar-01 commented Aug 14, 2021

Uh oh!

K-Kumar-01 commented Aug 17, 2021

Uh oh!

algomaster99 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

K-Kumar-01 commented Aug 17, 2021

Uh oh!

algomaster99 Aug 17, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

K-Kumar-01 Aug 17, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

K-Kumar-01 Aug 17, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

K-Kumar-01 commented Aug 10, 2021 •

edited

Loading

K-Kumar-01 commented Aug 12, 2021 •

edited

Loading

algomaster99 Aug 17, 2021 •

edited

Loading

K-Kumar-01 Aug 17, 2021 •

edited

Loading

K-Kumar-01 Aug 17, 2021 •

edited

Loading