-
-
Notifications
You must be signed in to change notification settings - Fork 72
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
Fix main page name for wikis with base URL rewritten #2025
Conversation
@TripleCamera Tahnk your for your PR and your patience, we will finally review your PR |
Hi @TripleCamera, thanks for the PR. Can you look into the failing tests? Also, please add additional tests for the code you changed, or otherwise verify that the code is covered by existing tests. Thanks! |
OK. I am running the tests on my computer. First, I need to run the
After fixing this, I will test my patch. |
It seems that you are in an area that network is restricted. You may need to use proxy. |
By the way, this pr is just a revert for 687b97ee. I think it is necessary to check this commit before merging this pr. |
Yes, I fixed this. Besides, thanks @XeroAlpha: The command of running a single test file should be Now I can pass the test without the patch, and fail the test with the patch. I printed out the value of
So I should submit another patch that replaces spaces with underscores in
Indeed. 687b97e belongs to #1197, which "should resolve #1180". I am getting confused:
@kelson42 Do you know the reason? |
Not sure what you mean here. I don't see any references to
What do you mean by "step"? I believe it was successfully removed.
According to https://www.mediawiki.org/wiki/API:Siteinfo :
The example is:
So this has the main page with the spaces already replaced. I believe the code was likely set up this way specifically so that spaces didn't have to be replaced. As you pointed out in #1995, MediaWiki sometimes reports that the "base" URL is not the "Main Page", which actually makes sense logically. Basically, I think you just need to do space replacement on your current PR and we should be good. Thanks again! |
I was talking about #1180. I couldn't understand kelson's description.
I inspected #1197 and couldn't understand why the "replace" step was removed, perhaps because the author thought they were unnecessary? Here is an example: @@ -96,11 +94,11 @@ class MediaWiki {
const entries = json.query[type];
Object.keys(entries).forEach((key) => {
const entry = entries[key];
- const name = entry['*'].replace(/ /g, self.spaceDelimiter);
+ const name = entry['*'];
const num = entry.id;
const allowedSubpages = ('subpages' in entry);
const isContent = !!(entry.content !== undefined || util.contains(addNamespaces, num));
- const canonical = entry.canonical ? entry.canonical.replace(/ /g, self.spaceDelimiter) : '';
+ const canonical = entry.canonical ? entry.canonical : '';
const details = { num, allowedSubpages, isContent };
/* Namespaces in local language */
self.namespaces[util.lcFirst(name)] = details; In my opinion, the author of the pull request simply needs to replace
Done. Now I can pass the tests on my computer. Please run Actions again. |
I think you're right, this is probably a bug, but I assume it doesn't affect the output ZIM. Re-running CI now. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
AFAIK we can not assume that articleIds replaces spaces by undescores, this depends how Mediwiki is configured. |
Actually, I can not find the reference anymore... |
This is a very small change that has been tested on my computer, so I'll just use the web editor.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #2025 +/- ##
==========================================
- Coverage 74.76% 71.93% -2.83%
==========================================
Files 41 41
Lines 3146 3146
Branches 689 689
==========================================
- Hits 2352 2263 -89
- Misses 675 748 +73
- Partials 119 135 +16 ☔ View full report in Codecov by Sentry. |
@audiodude This operation of replacing space by underscore is very standard. Very surprised there is no primitive already there for that. |
This is a very small change that has been tested on my computer, so I'll just use the web editor.
Fixes #1995.