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

feat: support include directive in GitHub issues / PRs / code blocks #422

Merged
merged 6 commits into from
Apr 13, 2020

Conversation

kumachan-mis
Copy link
Collaborator

@kumachan-mis kumachan-mis commented Apr 10, 2020

@kumachan-mis
Copy link
Collaborator Author

kumachan-mis commented Apr 10, 2020

Include directive in PR is also supported!
example https://github.com/WillBooster/plantuml-visualizer/pull/423/files

@kumachan-mis kumachan-mis changed the title feat: support include directive in GitHub file block feat: support include directive in GitHub file block and PR Apr 10, 2020
for (const finder of finders) {
for (const content of finder.find(webPageUrl, $root)) {
const $text = content.$text;
const arraysOfContents = await Promise.all(finders.map((finder) => finder.find(webPageUrl, $root)));
Copy link
Member

Choose a reason for hiding this comment

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

This statement waits all the promises, so I feel it is not good performance-wise.
Could we rewrite this as follows?

await Promise.all(finders.map(async (finder) => {
  const contents = await finder.find(webPageUrl, $root);
  for (const content of contents) {
    .. snip ..
  }
}));

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Shall I rewrite DiffMutator, too?

Copy link
Member

Choose a reason for hiding this comment

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

yes, please

Copy link
Member

@exKAZUu exKAZUu left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@exKAZUu exKAZUu changed the title feat: support include directive in GitHub file block and PR feat: support include directive in GitHub issues / PRs / code blocks Apr 13, 2020
@exKAZUu exKAZUu merged commit ecb43fc into master Apr 13, 2020
@exKAZUu exKAZUu deleted the 420_include_directive_FILE branch April 13, 2020 12:08
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.

2 participants