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

refactor: simplify blog url replacement regex #1491

Merged
merged 1 commit into from May 19, 2019

Conversation

Projects
None yet
5 participants
@Hongarc
Copy link
Contributor

commented May 18, 2019

Motivation

  • /\/index.html$/ is wrong, it should be /\/index\.html$/
  • Group /\/index\.html$/ and /\.html$/ to /(\/index)?\.html$/
  • Don't need changing new RegExp('/', 'g') but I think change it to same other RegExp
  • .replace('-', '/') was called thrice, maybe he want to replace all so I change it to /-/g (I'm wrong here)

Have you read the Contributing Guidelines on pull requests?

(Write your answer here.)

Test Plan

Input /(/index)?.html$/ //index.html$/ /.html$/
home/index.html home.md home.md
home/indexahtml home/indexahtml home.md
home.html home.md home.md

@Hongarc Hongarc requested a review from endiliey as a code owner May 18, 2019

@docusaurus-bot

This comment has been minimized.

Copy link
Collaborator

commented May 18, 2019

Deploy preview for docusaurus-2 ready!

Built with commit 49ea62e

https://deploy-preview-1491--docusaurus-2.netlify.com

@docusaurus-bot

This comment has been minimized.

Copy link
Collaborator

commented May 18, 2019

Deploy preview for docusaurus-preview ready!

Built with commit 49ea62e

https://deploy-preview-1491--docusaurus-preview.netlify.com

Show resolved Hide resolved packages/docusaurus-1.x/lib/server/blog.js Outdated
@yangshun
Copy link
Member

left a comment

Thanks for your PR, but a test plan of "Nah" is disrespectful to the project and the PR reviewers and can result in your future PRs being closed immediately.

@Hongarc Hongarc force-pushed the Hongarc:regex/blog branch from c355756 to 49ea62e May 19, 2019

@Hongarc

This comment has been minimized.

Copy link
Contributor Author

commented May 19, 2019

@yangshun
I add \ before . in regex so I think it doesn't need show test here.
Sorry about it, I will have the attention for the next pull request.

@yangshun yangshun changed the title Improve use replace method misc(v2): improve blog use replace method May 19, 2019

@yangshun
Copy link
Member

left a comment

Please read the CONTRIBUTING guidelines. It states that the PR titles have to follow a format. We would really appreciate if you read through the PR template more. I've changed it for you here.

I think this looks ok. cc @endiliey

@Hongarc Hongarc changed the title misc(v2): improve blog use replace method chore(v1): improve blog use replace method May 19, 2019

@Hongarc

This comment has been minimized.

Copy link
Contributor Author

commented May 19, 2019

@yangshun Thank you, This change for docusaurus-1.x so I change v2 to v1

@endiliey endiliey merged commit 9a18b74 into facebook:master May 19, 2019

3 checks passed

ci/circleci: lint-prettier Your tests passed on CircleCI!
Details
ci/circleci: tests Your tests passed on CircleCI!
Details
deploy/netlify Deploy preview ready!
Details

@endiliey endiliey changed the title chore(v1): improve blog use replace method refactor: simplify blog url replacement regex May 19, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.