Algoo ooxml #400

K-Kumar-01 · 2021-06-10T09:34:03Z

Changes

Adds the transformers for converting text and emphasize text (CiceroMarkJSON -> OOXML)
Adds the transformer for converting emphasize text(OOXML -> CiceroMarkJSON)
Test created for the above conversions.

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

Defines constructor Defines the function for converting CiceroMark to OOXML Signed-off-by: k-kumar-01 <[email protected]>

Removes contructor Nodes declared as class variable Signed-off-by: k-kumar-01 <[email protected]>

Rules and helpers for the conversion Conversion of text and emphasize in CiceroMarktoOOXML and for emphasize in OOXML transformer Tests for the above Signed-off-by: k-kumar-01 <[email protected]>

K-Kumar-01 · 2021-06-10T09:40:12Z

@algomaster99
I have made a PR. Let me know if there are any changes required.

Also a thing I want to ask. After the previous commit(June-5) was merged, I did the following:

  git checkout master
  git fetch --all --prune
  git rebase upstream/master
  git push origin master

And also git checkout algoo-ooxml and git rebase master. However, the old commits are still visible.
Is there something wrong which I followed.
Thanks in advance :)

algomaster99 · 2021-06-10T14:12:32Z

Is there something wrong which I followed.

Yes. git fetch only gets the new references but does not update your local repository. You need to merge it also. I advise you to instead use git pull which will merge your changes too.

I also want to bring a few more things related to your attention.

Is there a reason why you have named your branch algo-ooxml? AccordProject has laid out guidelines so you may follow them. However, the branch is anyway going to be deleted (no point to keep them after they are merged), so you may not strictly follow them but I would at least expect a short description there.
PR title is also very random. Your title should be very clear about what it does.
Link issues in your PR. It makes it easier to navigate across issues/pull requests. For example, CiceroMark<->OOXML transformers #397 should be linked in the PR description.

I get that all of this may sound time-consuming and you already have a lot to code but you need to understand that reviewing is equally tough if not more. Better PR title and better description tell the reviewer about what they can expect from the code beforehand. Thus, making it easier for one to review and perhaps, add more suggestions which would be a good thing for you.

K-Kumar-01 · 2021-06-10T14:29:55Z

Is there something wrong which I followed.

Yes. git fetch only gets the new references but does not update your local repository. You need to merge it also. I advise you to instead use git pull which will merge your changes too.

I also want to bring a few more things related to your attention.

Is there a reason why you have named your branch algo-ooxml? AccordProject has laid out guidelines so you may follow them. However, the branch is anyway going to be deleted (no point to keep them after they are merged), so you may not strictly follow them but I would at least expect a short description there.

PR title is also very random. Your title should be very clear about what it does.

Link issues in your PR. It makes it easier to navigate across issues/pull requests. For example, CiceroMark<->OOXML transformers #397 should be linked in the PR description.

I get that all of this may sound time-consuming and you already have a lot to code but you need to understand that reviewing is equally tough if not more. Better PR title and better description tell the reviewer about what they can expect from the code beforehand. Thus, making it easier for one to review and perhaps, add more suggestions which would be a good thing for you.

@algomaster99
The PR title was suggested so I didn't change it.
As of branch, I was working on algo-ooxml itself and not custom branch.
Last but not least, the issue is not closed completely by this PR. So I didn't mention it.

I will close this PR and create a new one from a newly created branch to algo-ooxml of upstream.
I apologize for the inconvenience caused.

algomaster99

See comments for detailed review. Make a habit of running npx eslint . --fix in markdown-docx before committing. We could configure hooks for it but let it be due to time constraints.

algomaster99 · 2021-06-10T14:14:55Z

packages/markdown-docx/src/CiceroMarkToOOXMLTransformer.test.js

+describe('Round Tripping', () => {
+
+    it('Pargaphs and emphasis', async () => {
+        const ciceroMarkJSON = {


You can put this entire JSON object in the test resources directory.

algomaster99 · 2021-06-10T14:18:04Z

packages/markdown-docx/src/CiceroMarkToOOXMLTransformer.test.js

+const OoxmlTransformer = require('./OoxmlTransformer');
+const CiceroMarkToOOXMLTransfomer = require('./CiceroMarkToOOXMLTransformer');
+
+describe('Round Tripping', () => {


You should be descriptive here. Maybe something like "perform roundtripping between CiceroMark and OOXML ". I can give you a reason for this as well. When you run the entire test suite, and suppose this fails, you will have this name handy to know where it failed.

algomaster99 · 2021-06-10T14:21:06Z

packages/markdown-docx/src/CiceroMarkToOOXMLTransformer.test.js

+
+describe('Round Tripping', () => {
+
+    it('Pargaphs and emphasis', async () => {


Reword this to should parse paragraphs and emphasis nodes.

Also, there is a typographical error in "paragraphs". If you are prone to them, I suggest you to install a VSCode extension, or if your computer specification allows, switch to an IDE like WebStorm.

algomaster99 · 2021-06-10T14:25:41Z

packages/markdown-docx/src/CiceroMarkToOOXMLTransformer.js

+        this.definedNodes = {
+            computedVariable: 'org.accordproject.ciceromark.ComputedVariable',
+            heading: 'org.accordproject.commonmark.Heading',
+            item: 'org.accordproject.commonmark.Item',
+            list: 'org.accordproject.commonmark.List',
+            listBlock: 'org.accordproject.ciceromark.ListBlock',
+            paragraph: 'org.accordproject.commonmark.Paragraph',
+            softbreak: 'org.accordproject.commonmark.Softbreak',
+            text: 'org.accordproject.commonmark.Text',
+            variable: 'org.accordproject.ciceromark.Variable',
+            emphasize: 'org.accordproject.commonmark.Emph',
+        };


Move this out of the class if eslint cannot parse this as a class variable.

algomaster99 · 2021-06-10T14:28:33Z

packages/markdown-docx/src/CiceroMarkToOOXMLTransformer.js

+     *
+     * @param {string} ooxml OOXML to be wrapped
+     */
+    wrapOOXML(ooxml){


These are exactly the kind of methods that should be in helpers./utils.

algomaster99 · 2021-06-10T14:35:27Z

Last but not least, the issue is not closed completely by this PR. So I didn't mention it.

You don't need to always prefix issues with Fix. You can just link it using Reference or without any prefix.

I apologize for the inconvenience caused.

You don't need to apologise. I am just telling you improvements on your PR and my intent is not to berate you. The monotonous nature of the text may say vice-versa but my sole purpose with such long texts is to help you understand and learn better.

algomaster99 · 2021-06-10T14:36:17Z

I just noticed you closed your PR. Anyway, do look at the reviews before submitting a new one.

K-Kumar-01 and others added 4 commits June 5, 2021 14:10

feat: sets-up class for CiceroMark to OOXML transformer

f0b6751

Defines constructor Defines the function for converting CiceroMark to OOXML Signed-off-by: k-kumar-01 <[email protected]>

feat: sets-up class for CiceroMark to OOXML transformer

6881d2a

Removes contructor Nodes declared as class variable Signed-off-by: k-kumar-01 <[email protected]>

feat: add text and emphasize converters

3e2a914

Rules and helpers for the conversion Conversion of text and emphasize in CiceroMarktoOOXML and for emphasize in OOXML transformer Tests for the above Signed-off-by: k-kumar-01 <[email protected]>

Merge branch 'algoo-ooxml' into algoo-ooxml

1a0a413

K-Kumar-01 requested a review from algomaster99 June 10, 2021 09:43

K-Kumar-01 closed this Jun 10, 2021

algomaster99 suggested changes Jun 10, 2021

View reviewed changes

K-Kumar-01 mentioned this pull request Jun 11, 2021

feat: add text and emphasis transformers - #397 #401

Merged

5 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Algoo ooxml #400

Algoo ooxml #400

K-Kumar-01 commented Jun 10, 2021

K-Kumar-01 commented Jun 10, 2021

algomaster99 commented Jun 10, 2021

K-Kumar-01 commented Jun 10, 2021

algomaster99 left a comment

algomaster99 Jun 10, 2021

algomaster99 Jun 10, 2021

algomaster99 Jun 10, 2021

algomaster99 Jun 10, 2021

algomaster99 Jun 10, 2021

algomaster99 commented Jun 10, 2021

algomaster99 commented Jun 10, 2021


		describe('Round Tripping', () => {

		it('Pargaphs and emphasis', async () => {

Algoo ooxml #400

Algoo ooxml #400

Conversation

K-Kumar-01 commented Jun 10, 2021

Changes

Author Checklist

K-Kumar-01 commented Jun 10, 2021

algomaster99 commented Jun 10, 2021

K-Kumar-01 commented Jun 10, 2021

algomaster99 left a comment

Choose a reason for hiding this comment

algomaster99 Jun 10, 2021

Choose a reason for hiding this comment

algomaster99 Jun 10, 2021

Choose a reason for hiding this comment

algomaster99 Jun 10, 2021

Choose a reason for hiding this comment

algomaster99 Jun 10, 2021

Choose a reason for hiding this comment

algomaster99 Jun 10, 2021

Choose a reason for hiding this comment

algomaster99 commented Jun 10, 2021

algomaster99 commented Jun 10, 2021