Skip to content

Allow an array of globs for the globs option#1118

Merged
marvinchin merged 1 commit intoMarkBind:masterfrom
ang-zeyu:list-of-globs
Mar 28, 2020
Merged

Allow an array of globs for the globs option#1118
marvinchin merged 1 commit intoMarkBind:masterfrom
ang-zeyu:list-of-globs

Conversation

@ang-zeyu
Copy link
Contributor

@ang-zeyu ang-zeyu commented Mar 11, 2020

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

• [x] Documentation update
• [x] Enhancement to an existing feature

Resolves #789

What is the rationale for this request?
Allow authors to specify an array of globs for the globs option, which can help reduce repeated configuration

What changes did you make? (Give an overview)

  • Simple one line code change to allow said feature
  • Small rewrite of the complex reduce - concat method of collecting pages with globs and walkSync using flatMap instead.
  • Some other minor refactors to collectAddressablePages
  • Update documentation accordingly
  • Update existing site.json for test sites / docs and update test files accordingly.
    • Two of the test sites' expected siteData.json's page entries order was swapped due to walkSync now collecting the pages from all globs in the array simultaneously. ( the properties remain the same though )

Provide some example code that this change will affect:

...
globs: Array.isArray(page.glob) ? page.glob : [page.glob],
...

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

  • Should we allow this for src as well?

Testing instructions:

  • npm run test should pass ( existing site.json for docs and test sites were updated to use new syntax )

Proposed commit message: (wrap lines at 72 characters)
Allow an array for specifying page src or glob

The site configuration does not allow using an array for specifying
pages in the src or glob property.
The method for collecting the addressable pages can also be
made slightly simpler in the process.

Let’s do so, reducing the amount of repeated configuration the
author potentially needs to write.
Let’s also simplify the method for collecting addressable pages
in the process.

@ang-zeyu ang-zeyu force-pushed the list-of-globs branch 3 times, most recently from 4a67349 to 10395a9 Compare March 23, 2020 09:54
@ang-zeyu
Copy link
Contributor Author

@yash-chowdhary could I get your review here?

The functional changes are summarised by this line
globs: Array.isArray(page.glob) ? page.glob : [page.glob],,
while the rest of the changes are just to refactor collectAddressablePages using lodash's flatMap.

@yash-chowdhary
Copy link
Contributor

  • Should we allow this for src as well?

Yes, I think this would be good. It would solve the same issue of repeated configuration. Unless there was a reason for making it a per-file configuration?
Maybe we can get the opinions of @damithc @marvinchin?

Otherwise, LGTM!

@damithc
Copy link
Contributor

damithc commented Mar 27, 2020

  • Should we allow this for src as well?

No objections from me

The site configuration does not allow using an array for specifying
pages in the src or glob property.
The method for collecting the addressable pages can also be
made slightly simpler in the process.

Let’s do so, reducing the amount of repeated configuration the
author potentially needs to write.
Let’s also simplify the method for collecting addressable pages
in the process.
Copy link
Contributor

@marvinchin marvinchin left a comment

Choose a reason for hiding this comment

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

Yeah, this looks good to me 🚀 Supporting an array of src sounds great as well! Let's do it in another PR?

@marvinchin marvinchin added this to the v2.12.1 milestone Mar 28, 2020
this.addressablePages = pages.filter(page => page.src);
const pagesFromSrc = _.flatMap(pages.filter(page => page.src), page => (Array.isArray(page.src)
? page.src.map(pageSrc => ({ ...page, src: pageSrc }))
: [page]));
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the feedback and reviews! @yash-chowdhary @damithc @marvinchin

Yeah, this looks good to me 🚀 Supporting an array of src sounds great as well! Let's do it in another PR?

My bad, pushed this before seeing this message. Its just a three line change here ^ though, let me know if it should be in a separate PR!

Copy link
Contributor

Choose a reason for hiding this comment

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

Its just a three line change here ^ though, let me know if it should be in a separate PR!

I think it can be left in this PR. 👍

@marvinchin marvinchin merged commit f7351af into MarkBind:master Mar 28, 2020
Tejas2805 added a commit to Tejas2805/markbind that referenced this pull request Mar 28, 2020
…bind into remove-fixed-bugs

* 'remove-fixed-bugs' of https://github.com/Tejas2805/markbind:
  Docs: Add contributing.md (MarkBind#1139)
  Show pointer and use underline dashed for click trigger (MarkBind#1111)
  Support variables to be defined as by JSON (MarkBind#1117)
  Allow an array for specifying page src or glob (MarkBind#1118)
  Code blocks: Implement inline markdown support for heading (MarkBind#1137)
  Fix lazy reload for urls without index (MarkBind#1110)
  Changes remaining references from filterTags to tags (MarkBind#1149)
marvinchin pushed a commit that referenced this pull request Apr 10, 2020
The site configuration does not allow using an array for specifying
pages in the src or glob property.
The method for collecting the addressable pages can also be
made slightly simpler in the process.

Let’s do so, reducing the amount of repeated configuration the
author potentially needs to write.
Let’s also simplify the method for collecting addressable pages
in the process.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

site.json: allow a list for glob

4 participants