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

🐛 rewrite '/'s in urls for Parsoid articles, not just MCS #954

Merged
merged 2 commits into from
Aug 29, 2019

Conversation

ISNIT0
Copy link
Contributor

@ISNIT0 ISNIT0 commented Aug 25, 2019

@kelson42
Copy link
Collaborator

  • No automated testing to avoid that for the nth time we have url rewriting problems
  • The way to rewrite URLs is really dirty, slow and highly probably buggy
    -> I don't want to release a new version of mwoffliner without part tested and written properly

@ISNIT0
Copy link
Contributor Author

ISNIT0 commented Aug 27, 2019

  • No automated testing to avoid that for the nth time we have url rewriting problems

You're right, this is something that should be tested with zimcheck, I'll add a customMainPage test.

  • The way to rewrite URLs is really dirty, slow and highly probably buggy
    -> I don't want to release a new version of mwoffliner without part tested and written properly

Although I agree that it'd be nice to replace the existing code with the native function, there is no evidence that the current logic is buggy, the recent re-open of #726 is nothing to do with the url handling logic, it's that the logic wasn't even applied to the HTML.
I don't want to make unnecessary changes in an already delayed release.

@codecov
Copy link

codecov bot commented Aug 27, 2019

Codecov Report

Merging #954 into master will not change coverage.
The diff coverage is 100%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #954   +/-   ##
=======================================
  Coverage   62.47%   62.47%           
=======================================
  Files          20       20           
  Lines        1671     1671           
  Branches      339      339           
=======================================
  Hits         1044     1044           
  Misses        457      457           
  Partials      170      170
Impacted Files Coverage Δ
src/util/rewriteUrls.ts 67.24% <100%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2618919...78d7bdd. Read the comment docs.

@ISNIT0 ISNIT0 requested a review from kelson42 August 28, 2019 08:41
@kelson42 kelson42 merged commit 79128ab into master Aug 29, 2019
@kelson42 kelson42 deleted the ISNIT0/parsoid-slash-rewriting branch August 29, 2019 18:05
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.

None yet

2 participants