Refactor preprocess and url processing functions #1026

ang-zeyu · 2020-02-05T11:03:15Z

What is the purpose of this pull request? (put "X" next to an item, remove the rest)

• [x] Other, please explain:

Part of #810

What is the rationale for this request?
Another step to modularising parser functions, enhancing code maintainability and readability.

What changes did you make? (Give an overview)

Break down _preprocess and move related functions to a separate file, componentPreprocessor
Refactor common ( to componentPreprocessor and parser ), stateless url processing functions to utils/urls
Enhance in-code documentation for the above along the way
Several smaller code refactors for the above ( reducing nested ifs, separating long functions when appropriate, etc. )

Provide some example code that this change will affect:

// directory structure from parser.js
- parser
  - utils
    - index
    - urls
  - preprocessors
    - componentPreprocessor
...

Is there anything you'd like reviewers to focus on?

Directory structure of the refactor
Of note is that componentPreprocessor and parser is still rather coupled. ( parser is passed as a parameter to the entry point in componentPreprocessor )

Ideally, only parser should depend on componentPreprocessor, and not the other way around at all if possible.
I initially tried making componentPreprocessor a class which would subsume required properties from the main parser. This was not feasible at the moment however, as many other parser functions have 'side effects' on parser's instance variables. It would be counterproductive to add code to synchronize the variables between the two.
Hence, we can proceed with a smaller refactor first, and gradually aim to remove this coupling if possible.

Testing instructions:
npm run test should pass

Proposed commit message: (wrap lines at 72 characters)
Refactor preprocess and url processing functions

The parser class houses a large number of functions.
This can be daunting for newer developers to the project, and decreases
maintainability of the code.

Let’s start by modularising the preprocess and url processing functions
into separate files.
Let’s also enhance some in-code documentation for these functions.
Some of these functions can also be abstracted into smaller units of
functionality, increasing the maintainability of the code.

ang-zeyu · 2020-02-07T10:05:08Z

Ready for review

Some quirks I noticed while refactoring which I'm not so sure about:

Boilerplate include sources are added to the parser's boilerplateIncludeSrc array. However, it is also added to dynamicIncludeSrc afterward, and is functionally the same for the purposes of live reloading. Should we remove boilerplateIncludeSrc? ( left as is for now )
_.hasIn is used quite a few times to check whether some attribute exists in an element. I've changed these to _.has, since attributes are direct properties of element.attribs, we do not want to check for indirect attributes.
Whether preprocessing logic for thumbnails ( moved to _preprocessThumbnail ) should be moved to componentParser instead ( left as is for now )
For include tags, there is some specific processing for context.mode. However, the _preprocess was never called with anything other than context.mode = 'include'. Should we remove this? ( left as is for now )
We only call _rebaseReferenceForStaticIncludes for <include>s
5.1. which's includeSrc is an md file, and
5.2. has a src that has some hash #....
Is this intentional? ( left as is for now ) ( though, changing it include 5.2 or both 5.2 and 5.1 and running updatetest didn't change any output test files )

Save for point 2 which is likely unintended, the rest of the refactor keeps the same functionality as before

ang-zeyu · 2020-02-07T10:23:11Z

src/lib/markbind/src/preprocessors/componentPreprocessor.js

+  console.warn(`<body> tag found in ${node.attribs[ATTRIB_CWF]}. This may cause formatting errors.`);
+}
+
+function preProcessComponent(node, context, config, parser) {


Entry point to the original _preprocess function, the respective switch cases are the respective control flows of _preprocess for different types of elements

ang-zeyu · 2020-02-07T10:23:59Z

src/lib/markbind/src/preprocessors/componentPreprocessor.js

+    }
+
+    // TODO clarify why this is only done if there is a hash, or fix it if it is unintended
+    if (hash) _rebaseReferenceForStaticIncludes(actualContent, element, config);


Is this intentional? ( point 5 )

ang-zeyu · 2020-02-07T10:25:42Z

src/lib/markbind/src/parser.js

-        element.attribs.boilerplate = element.attribs.boilerplate || path.basename(filePath);
-        actualFilePath
-          = Parser.calculateBoilerplateFilePath(element.attribs.boilerplate, filePath, config);
-        this.boilerplateIncludeSrc.push({ from: context.cwf, to: actualFilePath });


Is boilerplateIncludeSrc necessary?
For both panels and includes we eventually still push either dynamicIncludeSrc or staticIncludeSrc

ang-zeyu · 2020-02-07T10:26:13Z

src/lib/markbind/src/preprocessors/componentPreprocessor.js

+}
+
+// TODO move this to componentParser
+function _preProcessThumbnail(node) {


Should this be moved?

ang-zeyu · 2020-02-07T10:28:28Z

src/lib/markbind/src/preprocessors/componentPreprocessor.js

+      actualContent = isInline ? actualContent : `\n\n${actualContent}\n`;
+    } else {
+      // TODO do we need this branch?
+      actualContent = md.render(actualContent);


A quick usage find for _preprocess reveals that it is never called with anything other than context.mode = 'include'

ang-zeyu · 2020-02-07T10:41:16Z

src/lib/markbind/src/parser.js

-      if (element.children && element.children.length > 0) {
-        element.children = element.children.map(e => self._preprocess(e, context, config));
-      }
+  extractInnerVariablesIfNotProcessed(content, childContext, config, filePathToExtract) {


To allow componentPreprocessor and _extractInnerVariables to share this common code

ang-zeyu · 2020-02-07T10:44:50Z

src/lib/markbind/src/parser.js

-      this.preprocess(actualFilePath, pageData, context, config)
-        .then(resolve)
-        .catch(reject);
+      const currentContext = context;


This chunk is moved from the preprocess ( not _preprocess ) function, which was added in the expressive layouts pr,
to remove naming ambiguity.

A future PR may look into whether expressive layouts can be introduced in similar fashion to the page header, sitenav, etc., as includeData ( the function this code is under ) is very similar to includeFile. ( to reduce code duplication )

ang-zeyu · 2020-02-07T10:45:46Z

src/lib/markbind/src/parser.js

-      let newBase = Parser.calculateNewBaseUrls(filePath, this.rootPath, this.baseUrlMap);
-      if (newBase) {
-        const { relative, parent } = newBase;
+      let newBaseUrl = urlUtils.calculateNewBaseUrls(filePath, this.rootPath, this.baseUrlMap);


The only irrelevant refactor to remove the nested ifs, and slightly better variable naming; Let me know if this should be removed!

acjh · 2020-02-07T13:33:35Z

changing it ... and running updatetest didn't change any output test files

Instead of relying on non-exhaustive functional tests, for extensive refactoring, let's check that there is no diff to the generated _site of nus-cs2103/website-base.

ang-zeyu · 2020-02-07T19:38:11Z

Instead of relying on non-exhaustive functional tests, for extensive refactoring, let's check that there is no diff to the generated _site of nus-cs2103/website-base.

Thanks for the suggestion! Will take note of it for upcoming refactors.

I've removed the specific processing for context.mode, after confirming the output did not change between the latest master and my branch for the 2103 site. I added a console.log in the branch previously as well, and confirmed no statements were printed.

Interestingly, _rebaseReferenceForStaticIncludes with any combination of the conditions ( including removing it completely ) did not change the output.
I've left it in for now, it may be better to investigate this in more detail in a seperate PR.

ang-zeyu · 2020-02-11T11:06:19Z

Rebased on latest, no changes

Will keep this up to date, and update here if there are changes / conflicts

The parser class houses a large number of functions. This can be daunting for newer developers to the project, and decreases maintainability of the code. Let’s start by modularising the preprocess and url processing functions into separate files. Let’s also enhance some in-code documentation for these functions. Some of these functions can also be abstracted into smaller units of functionality, increasing the maintainability of the code.

yamgent · 2020-03-03T13:47:10Z

Should this be moved?

Whether it should be moved or not will depend on how specialised componentProcessor will eventually get. I noted that there's some coupling between this and the parser that still needs to be resolved in future PRs, so I believe the answer will naturally come by as the refactoring process goes.

Also noted about the weird quirks that you have found. Refactoring by itself is usually quite a tough task by itself; verifying additional changes on top of it is going to make it even more tough, so always welcome to settle those problems in separate PRs (and good to open these in the issue tracker too).

Since my time is now quite restricted as compared to the past, I am no longer able to manually verify that everything works. So these green ticks approval that I give from now on would basically be just a glance through your code to see that there's no major source of issues in terms of design and implementation. You guys might want to test out each other's code from time to time to see if there's any problems.

With all that said, will be merging this. 👍

…nvert-to-code-block * 'master' of https://github.com/MarkBind/markbind: Allow changing parameter properties (MarkBind#1075) Custom timezone for built-in timestamp (MarkBind#1073) Fix reload inconsistency when updating frontmatter (MarkBind#1068) Implement an api to ignore content in certain tags (MarkBind#1047) Enable AppVeyor CI (MarkBind#1040) Add heading and line highlighting to code blocks (MarkBind#1034) Add dividers and fix bug in siteNav (MarkBind#1063) Fixed navbar no longer covers modals (MarkBind#1070) Add copy code-block plugin (MarkBind#1043) Render plugins on dynamic resources (MarkBind#1051) Documentation for Implement no-* attributes for <box> (MarkBind#1042) Migrate to bootstrap-vue popovers (MarkBind#1033) Refactor preprocess and url processing functions (MarkBind#1026) Add pageNav to Using Plugins Page (MarkBind#1062) # Conflicts: # docs/userGuide/syntax/siteNavigationMenus.mbdf

* 'master' of https://github.com/MarkBind/markbind: 2.12.0 Update outdated test files Update vue-strap version to v2.0.1-markbind.37 Fix refactor to processDynamicResources (MarkBind#1092) Implement lazy page building for markbind serve (MarkBind#1038) Add warnings for conflicting/deprecated component attribs (MarkBind#1057) Allow changing parameter properties (MarkBind#1075) Custom timezone for built-in timestamp (MarkBind#1073) Fix reload inconsistency when updating frontmatter (MarkBind#1068) Implement an api to ignore content in certain tags (MarkBind#1047) Enable AppVeyor CI (MarkBind#1040) Add heading and line highlighting to code blocks (MarkBind#1034) Add dividers and fix bug in siteNav (MarkBind#1063) Fixed navbar no longer covers modals (MarkBind#1070) Add copy code-block plugin (MarkBind#1043) Render plugins on dynamic resources (MarkBind#1051) Documentation for Implement no-* attributes for <box> (MarkBind#1042) Migrate to bootstrap-vue popovers (MarkBind#1033) Refactor preprocess and url processing functions (MarkBind#1026) Add pageNav to Using Plugins Page (MarkBind#1062)

The parser class houses a large number of functions. This can be daunting for newer developers to the project, and decreases maintainability of the code. Let’s start by modularising the preprocess and url processing functions into separate files. Let’s also enhance some in-code documentation for these functions. Some of these functions can also be abstracted into smaller units of functionality, increasing the maintainability of the code.

ang-zeyu force-pushed the refactor-urls-and-preprocessing branch 8 times, most recently from e56c14f to 016d781 Compare February 7, 2020 09:51

ang-zeyu changed the title ~~[WIP] Refactor preprocess and url processing functions~~ Refactor preprocess and url processing functions Feb 7, 2020

ang-zeyu commented Feb 7, 2020

View reviewed changes

ang-zeyu force-pushed the refactor-urls-and-preprocessing branch from 016d781 to 4f735f7 Compare February 7, 2020 10:40

ang-zeyu commented Feb 7, 2020

View reviewed changes

ang-zeyu force-pushed the refactor-urls-and-preprocessing branch 4 times, most recently from 5b18ab0 to b33008b Compare February 7, 2020 19:35

ang-zeyu mentioned this pull request Feb 8, 2020

{{baseUrl}} in boilerplates is not behaving as an absolute URL #928

Closed

ang-zeyu force-pushed the refactor-urls-and-preprocessing branch from b33008b to ae39616 Compare February 11, 2020 10:59

ang-zeyu force-pushed the refactor-urls-and-preprocessing branch from ae39616 to c647c49 Compare February 15, 2020 05:29

ang-zeyu mentioned this pull request Feb 15, 2020

Inline PUML #968

Closed

ang-zeyu force-pushed the refactor-urls-and-preprocessing branch 3 times, most recently from ac1a05d to 9644a60 Compare February 24, 2020 12:55

ang-zeyu force-pushed the refactor-urls-and-preprocessing branch from 9644a60 to d2a24dd Compare February 26, 2020 10:24

ang-zeyu requested a review from yamgent February 29, 2020 14:56

yamgent approved these changes Mar 3, 2020

View reviewed changes

yamgent added this to the v2.11.1 milestone Mar 3, 2020

yamgent merged commit 2eb7b7d into MarkBind:master Mar 3, 2020

yamgent added the pr.CodeMaintenance 🛠 DevOps, refactoring, etc label Mar 3, 2020

This was referenced Mar 4, 2020

Investigate removing boilerplateIncludeSrc #1081

Closed

Investigate moving preprocessThumbnails to componentParser #1082

Closed

Investigate the baseUrl resolving process as a whole #1083

Closed

Investigate removing context.mode completely #1084

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor preprocess and url processing functions #1026

Refactor preprocess and url processing functions #1026

ang-zeyu commented Feb 5, 2020 •

edited

Loading

ang-zeyu commented Feb 7, 2020 •

edited

Loading

ang-zeyu Feb 7, 2020

ang-zeyu Feb 7, 2020

ang-zeyu Feb 7, 2020

ang-zeyu Feb 7, 2020

ang-zeyu Feb 7, 2020

ang-zeyu Feb 7, 2020

ang-zeyu Feb 7, 2020

ang-zeyu Feb 7, 2020

acjh commented Feb 7, 2020

ang-zeyu commented Feb 7, 2020

ang-zeyu commented Feb 11, 2020 •

edited

Loading

yamgent commented Mar 3, 2020

Refactor preprocess and url processing functions #1026

Refactor preprocess and url processing functions #1026

Conversation

ang-zeyu commented Feb 5, 2020 • edited Loading

ang-zeyu commented Feb 7, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

acjh commented Feb 7, 2020

ang-zeyu commented Feb 7, 2020

ang-zeyu commented Feb 11, 2020 • edited Loading

yamgent commented Mar 3, 2020

ang-zeyu commented Feb 5, 2020 •

edited

Loading

ang-zeyu commented Feb 7, 2020 •

edited

Loading

ang-zeyu commented Feb 11, 2020 •

edited

Loading