Automatically convert GitHub-based projects into MarkBind websites #698

amad-person · 2019-02-17T14:03:58Z

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

• [X] New feature

Fixes #675. Part of #660.

What is the rationale for this request?

Provides a way to convert an existing Github wiki or a docs folder into a MarkBind website. It will be easier to integrate MarkBind into the workflow for Github-based project.

What changes did you make? (Give an overview)

Added a new option for the init command: --convert/-c. Users can run markbind init -c in the desired directory to convert it into a MarkBind project.
Converting the folder into a MarkBind project includes the following:
- Running a usual markbind init.
- Then, copying over contents of README.md (or Home.md) to index.md
- Adding an about.md page if not present.
- Adding a site navigation menu to all pages by updating navigation.md in the default layout. As mentioned in Automatically convert GitHub Wikis to MarkBind sites #675, if _Sidebar.md is present in the root directory, this will be used for the site navigation menu. Otherwise a menu will be generated.
- Adding a footer to the website by updating footer.md in the default layout. As mentioned in Automatically convert GitHub Wikis to MarkBind sites #675, if _Footer.md is present in the root directory, this will be used for the site navigation menu. Otherwise the default footer generated by markbind init will be used.
- Adding a top navigation bar to all pages. This is done by adding a nav bar into header.md in the default layout.
- Updating site config so all pages have the default layout applied to them.

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

Correctness of test.bat file.

Testing instructions:

Run markbind init -c in a folder of your choice and build the site. Run npm run test(win) to run the UTs and FT.

Proposed Commit Message:

Automatically convert GitHub-based projects into MarkBind websites

GitHub projects commonly have wikis or docs folders for project
documentation. As MarkBind is especially optimized as a project
documentation tool, we would want to make it easier for projects to
adopt it in their workflow.

Let's add the ability to automatically convert a GitHub wiki or docs
folder into a MarkBind website.

amad-person · 2019-03-02T10:15:31Z

I'm not sure where to document this functionality. Should I add information in MarkBind in the Project Workflow and include a link to the section in CLI Commands?
For testing this, should I add a dummy folder and provide an expected output folder (similar to our functional testing)?

damithc · 2019-03-02T10:41:55Z

I'm not sure where to document this functionality. Should I add information in MarkBind in the Project Workflow and include a link to the section in CLI Commands?

That sounds about right.

src/Site.js

nicholaschuayunzhi

Thanks for the PR @amad-person. I've left some comments and potential improvements. Overall, I think the process is sound and I've tested it on some wikis. I think the thing that bugs me the most is including the header in every addressable page. Not sure if layouts/front matter can help us here.

Do remember to write the documentation and tests! When you're done remember to remove the [WIP] tag in the PR title. Once again thanks for the effort!

src/Site.js

Xenonym · 2019-03-04T08:43:47Z

If you try --convert with zaproxy/zaproxy/wiki, it will fail out of the box as the page FAQformauth contains the text username={%username%}&password={%password%}, which Nunjucks tries and fails to parse as variables:

error: uncaughtException: (C:\Users\user\Desktop\zaproxy.wiki\FAQformauth.md) [Line 31, Column 114]
  unknown block tag: username date=Mon Mar 04 2019 16:21:20 GMT+0800 (Malay Peninsula Standard Time), pid=27616, uid=null, gid=null, cwd=C:\Users\user\Desktop\zaproxy.wiki, execPath=C:\Program Files\nodejs\node.exe, version=v8.15.0, argv=[C:\Program Files\nodejs\node.exe, C:\Program Files\nodejs\node_modules\markbind-cli\index.js, serve], rss=90669056, heapTotal=78016512, heapUsed=46236248, external=927768, loadavg=[0, 0, 0], uptime=142101
Template render error: (C:\Users\user\Desktop\zaproxy.wiki\FAQformauth.md) [Line 31, Column 114]
  unknown block tag: username
    at Object.exports.prettifyError (C:\Users\user\Documents\GitHub\CS3281-2\markbind\node_modules\nunjucks\src\lib.js:34:15)
    at new_cls.render (C:\Users\user\Documents\GitHub\CS3281-2\markbind\node_modules\nunjucks\src\environment.js:472:27)
    at new_cls.renderString (C:\Users\user\Documents\GitHub\CS3281-2\markbind\node_modules\nunjucks\src\environment.js:328:21)
    at Object.module.exports.renderString (C:\Users\user\Documents\GitHub\CS3281-2\markbind\node_modules\nunjucks\index.js:80:14)
    at fs.readFile (C:\Users\user\Documents\GitHub\CS3281-2\markbind\src\lib\markbind\src\parser.js:508:36)
    at FSReqWrap.readFileAfterClose [as oncomplete] (fs.js:511:3)

We should either parse the wiki pages to "escape" problematic syntax, or at least document that --convert may not work if there is MarkBind/Nunjucks syntax in wiki pages.

yamgent · 2019-03-06T03:47:40Z

We should either parse the wiki pages to "escape" problematic syntax, or at least document that --convert may not work if there is MarkBind/Nunjucks syntax in wiki pages.

I think in general, when we fail a conversion, we should just retain the original .md file content, but print a warning to say that we have failed for this .md, and continue on with our business.

For edge cases in general, we can try to apply fixes wherever possible, a good rule of thumb is that if the fix is not too complicated in code, and the fix is not "controversial" in nature, then we can consider handling the edge case. Otherwise we shouldn't worry about them too much.

For this edge case though, while I think escaping could be a good try, the problem is that there is no known consistent way of escaping Nunjucks stuff (this is a known bug btw, we have always been abusing <span> in the past to get around the problem of Nunjucks interpreting stuff that is not supposed to be interpreted as Nunjucks code).

amad-person · 2019-03-07T07:35:18Z

Currently, the conversion process doesn't parse the .md files, so errors (if any) will arise only on markbind build.

Any ideas for getting these build errors? Maybe I can parse the files before starting the conversion and log the errors - so users can fix them and start again?

nicholaschuayunzhi · 2019-03-07T08:28:45Z

Personally I feel that if we handle nunjucks error well, the author would be able to identify the problem and adjust accordingly? Perhaps this isn't in the scope of this feature? @yamgent Do you agree?

yamgent · 2019-03-07T08:46:05Z

Personally I feel that if we handle nunjucks error well, the author would be able to identify the problem and adjust accordingly? Perhaps this isn't in the scope of this feature? @yamgent Do you agree?

I agree that this PR shouldn't be going inside the nunjucks code and modify stuff for this current edge case, the fixing of the errors should be done manually by the author. Printing a user-friendly message will make that experience better, so I agree that this isn't the scope of the current PR.

amad-person · 2019-03-08T08:33:14Z

So, would adding the following in this PR be okay?

Printing a message to the console

--convert may not work if there is MarkBind/Nunjucks syntax

(for all cases, whether there might be build errors or not)
Adding this warning to the documentation.

yamgent · 2019-03-08T08:39:37Z

Printing a message to the console

--convert may not work if there is MarkBind/Nunjucks syntax

(for all cases, whether there might be build errors or not)

I don't think this message would be very useful, such messages are generally not paid attention to, because it works, the error message is redundant, and if it doesn't, users will see the error churned out by nunjucks and may not make the connections between the error and this message.

The documentation is fine and we should definitely include it.

docs/userGuide/markBindInTheProjectWorkflow.md

src/Site.js

amad-person · 2019-03-14T23:15:00Z

Updates after discussion -
Print a message to let users know they can update {{ baseUrl }}.
Two formats of site navigation files need to be supported:

[[Name|URL]]
[Name](URL)

src/util/fsUtil.js

src/Site.js

src/util/fsUtil.js

src/Site.js

nicholaschuayunzhi

Thanks for the changes @amad-person. I've left some brief comments on the things we went through during the meeting.

I also think that you have a misunderstanding on how some of the js features work. I've left some comments wrt to one code snippet. Please let me know if it was unclear.

Also do remember to write tests and remove [WIP] in your PR title when you are ready for review.

src/Site.js

nicholaschuayunzhi · 2019-03-28T07:40:00Z

src/Site.js

+            const updatedAddressablePageContent = includeTopNavCode + addressablePageContent;
+            return fs.outputFileAsync(addressablePagePath, updatedAddressablePageContent);
+          }
+          return Promise.all(outputFilesPromises);


I assume what you are trying to do is to create an array of promises called outputFilesPromises, after which calling Promise.all(outputFilesPromises so that when all pages have been written, it will continue down the promise chain.

I think there are 2 main problems with this code.

forEach (line 598)

outputFilesPromises will be undefined because forEach would return undefined

you should use map instead

Promise.all (line 607)

Promise.all takes in an array (iterable) of promises. Only when all the promises are resolved would it resolve and continue down the promise chain.

returning this in the for loop does not do any assignment

does not pass this promise down the chain. As such, the next .then will execute before all the files have been output.

i believe the code that should be written is

const outputFilesPromises = this.addressablePages .filter(addressablePage => !addressablePage.src.startsWith('_')) .map((page) => { const addressablePagePath = path.join(this.rootPath, page.src); const topNavRelativePath = path.relative(this.rootPath, topNavDefaultPath); if (topNavRelativePath.length > 0) { const includeTopNavCode = `<include src="${topNavRelativePath}" />\n\n`; const addressablePageContent = fs.readFileSync(addressablePagePath, 'utf8'); const updatedAddressablePageContent = includeTopNavCode + addressablePageContent; return fs.outputFileAsync(addressablePagePath, updatedAddressablePageContent); } return Promise.resolve(); }); return Promise.all(outputFilesPromises);

In addition, some statements can be moved out of map.

const topNavRelativePath = path.relative(this.rootPath, topNavDefaultPath); if (topNavRelativePath.length == 0) { return Promise.resolve(); } const includeTopNavCode = `<include src="${topNavRelativePath}" />\n\n`; const outputFilesPromises = this.addressablePages .filter(addressablePage => !addressablePage.src.startsWith('_')) .map((page) => { const addressablePagePath = path.join(this.rootPath, page.src); const addressablePageContent = fs.readFileSync(addressablePagePath, 'utf8'); const updatedAddressablePageContent = includeTopNavCode + addressablePageContent; return fs.outputFileAsync(addressablePagePath, updatedAddressablePageContent); }); return Promise.all(outputFilesPromises);

This function modifies the source files?

This function inserts a top nav to every addressable page. I dont think there's a clean way to insert a top nav without modifying the original wiki site in some way.

Oh, this is specific to conversion. If not entirely possible to abstract this out of Site, then shall we at least put this in a different file (e.g. Site.convert.js)? We can expect the caller to require that file after Site.js.

@amad-person could you update the feature based on #698 (comment) ?

@acjh @yamgent how should we handle the constants in Site.js used in the conversion feature. It seems like defining constants in that manner is not good for modularity.

Should we:

Make the used constants static variables of Site

Copy the constants in Site.convert.js and refactor in the future

Keep the feature in Site and refactor everything in one shot

Copy the constants in Site.convert.js and refactor in the future

This seems like a pretty good option

Projects usually have file(s) for constants, e.g. https://github.com/vuejs/vue/blob/dev/src/shared/constants.js

Let's start with src/constants.js.

Thanks for the input @acjh @yamgent. From discussion today, we already have an issue tracking this #597. Perhaps the change is out of scope of the pull request? Perhaps we shall do all the refactoring in one shot and when there is less on going development at the moment.

So to be clear what i mean is that the conversion logic will be be in Site.js.

I am cool with it.

test/unit/Site.test.js

nicholaschuayunzhi

Thanks for the changes and sorry for late review. I noted that if my test fails, the removal of unwanted directories is not done. Additionally I've left some comments.

As for the styling of the side nav and footer I agree that it can be done in another PR and is also a candidate for good first issue.

test/functional/test.sh

test/functional/test.bat

nicholaschuayunzhi · 2019-04-10T15:18:09Z

test/unit/Site.test.js

+  expect(fs.existsSync(path.resolve('inner/_markbind/layouts/default/footer.md'))).toEqual(true);
+
+  // site navigation
+  expect(fs.existsSync(path.resolve('inner/_markbind/layouts/default/navigation.md'))).toEqual(true);


Was this addressed else where?

If no I was thinking of doing a check on the content of the navigation file here. The file is generated by the conversion and the results should be predictable.

docs/userGuide/cliCommands.md

* Add section in Project Workflow page * Add link to section in CLI Commands page

* Minor changes to documentation Co-Authored-By: amad-person <aadyaa@u.nus.edu>

…ailure

…failure

nicholaschuayunzhi · 2019-04-12T01:22:15Z

@amad-person thanks for the PR. Could you write a a Proposed Commit message for the PR (to be updated in the PR description)?

amad-person · 2019-04-12T03:33:31Z

@nicholaschuayunzhi @yamgent updated PR description with the proposed commit message.

amad-person force-pushed the add-convert-wiki branch from 31fa32c to dafe7a4 Compare March 2, 2019 09:37

amad-person marked this pull request as ready for review March 2, 2019 10:06

nicholaschuayunzhi self-requested a review March 3, 2019 08:27

nicholaschuayunzhi reviewed Mar 3, 2019

View reviewed changes

src/Site.js Outdated Show resolved Hide resolved

src/Site.js Outdated Show resolved Hide resolved

src/Site.js Outdated Show resolved Hide resolved

nicholaschuayunzhi suggested changes Mar 3, 2019

View reviewed changes

acjh reviewed Mar 3, 2019

View reviewed changes

src/Site.js Outdated Show resolved Hide resolved

src/Site.js Outdated Show resolved Hide resolved

src/Site.js Outdated Show resolved Hide resolved

amad-person changed the title ~~[WIP] Add ability to convert Github wiki to MarkBind site~~ [WIP] Add ability to convert Github wiki/docs to MarkBind site Mar 7, 2019

yamgent force-pushed the master branch from 7f35131 to 43a048d Compare March 8, 2019 15:01

amad-person force-pushed the add-convert-wiki branch from cf4de8a to 258ad1e Compare March 14, 2019 06:44

Xenonym reviewed Mar 14, 2019

View reviewed changes

marvinchin reviewed Mar 17, 2019

View reviewed changes

src/util/fsUtil.js Outdated Show resolved Hide resolved

yamgent reviewed Mar 18, 2019

View reviewed changes

src/Site.js Outdated Show resolved Hide resolved

src/Site.js Outdated Show resolved Hide resolved

src/Site.js Show resolved Hide resolved

src/Site.js Outdated Show resolved Hide resolved

yamgent reviewed Mar 20, 2019

View reviewed changes

src/util/fsUtil.js Outdated Show resolved Hide resolved

src/Site.js Outdated Show resolved Hide resolved

src/Site.js Outdated Show resolved Hide resolved

amad-person force-pushed the add-convert-wiki branch 2 times, most recently from 020c49d to faeab48 Compare March 26, 2019 11:50

amad-person requested review from yamgent and nicholaschuayunzhi March 28, 2019 06:17

nicholaschuayunzhi suggested changes Mar 28, 2019

View reviewed changes

amad-person force-pushed the add-convert-wiki branch from 7910455 to 22707f0 Compare March 29, 2019 14:13

nicholaschuayunzhi reviewed Mar 30, 2019

View reviewed changes

test/unit/Site.test.js Outdated Show resolved Hide resolved

nicholaschuayunzhi suggested changes Apr 10, 2019

View reviewed changes

amad-person dismissed stale reviews from yamgent via c01a988 April 10, 2019 15:39

amad-person and others added 15 commits April 11, 2019 15:32

Add initial prototype of wiki conversion

aab2b08

Add documentation for the conversion feature

d27f30d

* Add section in Project Workflow page * Add link to section in CLI Commands page

Update after review

601c057

Use header in layout instead of including top nav file in pages

f12f8f2

Add unit tests

9936675

Add functional test

c469a69

Update after review

0688682

Bug fix: conversion crashed when Home.md or README.md were not present

1bbfdd2

Reword adding About page documentation

8ccc077

Bug fix: abort conversion on existing MarkBind sites

8e05778

Apply suggestions from code review

92a521c

* Minor changes to documentation Co-Authored-By: amad-person <aadyaa@u.nus.edu>

Bug fix: test.sh cleans up conversion files generated in FT even on f…

ca3508e

…ailure

Bug fix: test.bat cleans up conversion files generated in FT even on …

b0ee811

…failure

Update test_site_convert/expected after rebase

ca1fb8b

Fix test.bat

93cb7e4

amad-person force-pushed the add-convert-wiki branch from b4b602a to 93cb7e4 Compare April 11, 2019 08:13

amad-person added 2 commits April 11, 2019 16:19

Fix test.bat

c00f757

Fix eslint error

3f4440e

yamgent mentioned this pull request Apr 11, 2019

Add script to update expected folder for test sites #828

Merged

nicholaschuayunzhi approved these changes Apr 11, 2019

View reviewed changes

yamgent approved these changes Apr 12, 2019

View reviewed changes

yamgent added this to the v2.1.2 milestone Apr 12, 2019

yamgent changed the title ~~Add ability to convert Github wiki/docs to MarkBind site~~ Automatically convert GitHub-based projects into MarkBind websites Apr 12, 2019

yamgent merged commit db4ce52 into MarkBind:master Apr 12, 2019

amad-person deleted the add-convert-wiki branch December 7, 2019 07:27

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Automatically convert GitHub-based projects into MarkBind websites #698

Automatically convert GitHub-based projects into MarkBind websites #698

amad-person commented Feb 17, 2019 •

edited

Loading

amad-person commented Mar 2, 2019

damithc commented Mar 2, 2019

nicholaschuayunzhi left a comment

Xenonym commented Mar 4, 2019

yamgent commented Mar 6, 2019

amad-person commented Mar 7, 2019

nicholaschuayunzhi commented Mar 7, 2019

yamgent commented Mar 7, 2019

amad-person commented Mar 8, 2019

yamgent commented Mar 8, 2019

amad-person commented Mar 14, 2019

nicholaschuayunzhi left a comment •

edited

Loading

nicholaschuayunzhi Mar 28, 2019

acjh Mar 28, 2019

nicholaschuayunzhi Mar 29, 2019

acjh Mar 29, 2019

nicholaschuayunzhi Mar 30, 2019

nicholaschuayunzhi Mar 30, 2019

yamgent Mar 30, 2019

acjh Apr 1, 2019

nicholaschuayunzhi Apr 4, 2019

yamgent Apr 4, 2019

nicholaschuayunzhi left a comment

nicholaschuayunzhi Apr 10, 2019

nicholaschuayunzhi commented Apr 12, 2019

amad-person commented Apr 12, 2019

Automatically convert GitHub-based projects into MarkBind websites #698

Automatically convert GitHub-based projects into MarkBind websites #698

Conversation

amad-person commented Feb 17, 2019 • edited Loading

amad-person commented Mar 2, 2019

damithc commented Mar 2, 2019

nicholaschuayunzhi left a comment

Choose a reason for hiding this comment

Xenonym commented Mar 4, 2019

yamgent commented Mar 6, 2019

amad-person commented Mar 7, 2019

nicholaschuayunzhi commented Mar 7, 2019

yamgent commented Mar 7, 2019

amad-person commented Mar 8, 2019

yamgent commented Mar 8, 2019

amad-person commented Mar 14, 2019

nicholaschuayunzhi left a comment • 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nicholaschuayunzhi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nicholaschuayunzhi commented Apr 12, 2019

amad-person commented Apr 12, 2019

amad-person commented Feb 17, 2019 •

edited

Loading

nicholaschuayunzhi left a comment •

edited

Loading