Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improve the default site.json settings generated during init #525

Merged
merged 1 commit into from Jan 13, 2019

Conversation

amad-person
Copy link
Contributor

@amad-person amad-person commented Jan 3, 2019

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

• [X] Enhancement to an existing feature

Fixes #507.
Fixes #563.

What is the rationale for this request?

As per #507, the default site.json generated during markbind init can be improved by serving all .md and .mbd files by default (and ignoring .mbd files and the .git directory). Hence users need to tweak site.json only when they require some additional setup, e.g. overriding a page title.

What changes did you make? (Give an overview)

Updated the default site.json that is generated by the markbind init command as follows:

  1. Added *.md and *.mbd to the pages array so that all .md and .mbd files will be served by default.
  2. Added *.mbd and .git/* to the ignore array so that these files will be ignored while copying over to the generated site's directory.

Provide some example code that this change will affect:

Default site.json generated by markbind init after this change:

{
  "baseUrl": "",
  "titlePrefix": "",
  "ignore": [
    "_markbind/layouts/*",
    "_markbind/logs/*",
    "_site/*",
    "site.json",
    "*.md",
    "*.mbd",
    ".git/*"
  ],
  "pages": [
    {
      "src": "index.md",
      "title": "Hello World"
    },
    {
      "glob": "**/index.md"
    },
    {
      "glob": "**/*.+(md|mbd)"
    }
  ],
  "deploy": {
    "message": "Site Update."
  }
}

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

N.A.

Testing instructions:

Run the test suite / build markbind locally and test.

@Xenonym
Copy link
Contributor

Xenonym commented Jan 3, 2019

  1. Would it make sense to ignore .gitignore as well since we are already ignoring .git/*?
  2. The glob patterns *.md and *.mbd would only match .md and .mbd files in the root directory of the site eg. inside/doc.md would not be matched and rendered. Is this the intended behavior?

@amad-person
Copy link
Contributor Author

amad-person commented Jan 3, 2019

For the second point - would updating the patterns to **/*.md and **/*.mbd work?

@Xenonym
Copy link
Contributor

Xenonym commented Jan 3, 2019

@amad-person they should, but it doesn't seem to work in testing, see #526.

EDIT: I goofed, the above mentioned patterns should work.

@acjh
Copy link
Contributor

acjh commented Jan 3, 2019

Why are .md and .mbd in both ignore and glob?

@amad-person
Copy link
Contributor Author

@acjh from what I understand, the requirement is that all .md and .mbd files should be rendered as HTML files i.e. they should be in glob. But the original markdown files shouldn't be copied over to the generated site directory, so .md and .mbd are in ignore also.

@Xenonym
Copy link
Contributor

Xenonym commented Jan 4, 2019

@amad-person **/*.md and **/.*mbd would work! In fact, you could combine them into one pattern: **/*.+(md|mbd).

However, this would also render all files in the _markbind directory eg. _markbind/variables.md, which I am not sure is useful. {,!(_markbind)/**/}*.+(md|mbd) would exclude those files, but it causes the unit test to fail, so I suspect that's intended behaviour?

@yamgent
Copy link
Member

yamgent commented Jan 5, 2019

However, this would also render all files in the _markbind directory eg. _markbind/variables.md, which I am not sure is useful. {,!(_markbind)/**/}*.+(md|mbd) would exclude those files, but it causes the unit test to fail, so I suspect that's intended behaviour?

There's currently no way to exclude pages, and the current codebase doesn't explicitly exclude the _markbind/ directory as well. So the behaviour is expected, but I don't think it is a desirable behaviour.

It should be possible to correct the behaviour in Site.prototype.collectAddressablePages, no need to resort to complicated regex for now. :P

@amad-person
Copy link
Contributor Author

I have updated to glob syntax to serve all files, please let me know if I need to make any changes!

Copy link
Member

@yamgent yamgent left a comment

Choose a reason for hiding this comment

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

We still need to exclude the files inside _markbind/, by doing the following:

It should be possible to correct the behaviour in Site.prototype.collectAddressablePages

@amad-person
Copy link
Contributor Author

@yamgent

For excluding the files inside _markbind can I change the value of BOILERPLATE_FOLDER_NAME (Site.js#28) to '_markbind'? The current value of this is '_markbind/boilerplates' and it is being used to ignore the boilerplates folder as given in Site.js#367.

@acjh
Copy link
Contributor

acjh commented Jan 6, 2019

No, boilerplate files are a feature in MarkBind and that constant is used in other places.

src/Site.js Outdated
@@ -364,7 +365,7 @@ Site.prototype.collectAddressablePages = function () {
globPages.concat(walkSync(this.rootPath, {
directories: false,
globs: [addressableGlob.glob],
ignore: [BOILERPLATE_FOLDER_NAME],
ignore: [BOILERPLATE_FOLDER_NAME, CONFIG_FOLDER_NAME],
Copy link
Contributor

Choose a reason for hiding this comment

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

Can remove BOILERPLATE_FOLDER_NAME since it's a subdirectory of CONFIG_FOLDER_NAME?

Copy link
Member

@yamgent yamgent left a comment

Choose a reason for hiding this comment

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

Nice work. Please squash the commits! Thank you.

* serve all .mbd and .md files
* ignore all .mbd files and the .git folder
@yamgent yamgent added this to the v1.16.2 milestone Jan 13, 2019
@yamgent yamgent merged commit ee30204 into MarkBind:master Jan 13, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants