-
Notifications
You must be signed in to change notification settings - Fork 12
Domain functionality #39
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
Conversation
|
My initial pull request had #40 mixed in and I split those functionalities now. |
yamdwe.py
Outdated
| print("Found %d pages to export..." % len(pages)) | ||
|
|
||
| # Add a shameless "exported by yamdwe" note to the front page of the wiki | ||
| # Add a shameless "exported by yamdwe" note to the front page of the wiki - really shameless, but I'll keep it |
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.
If the note really bugs you then feel free to send a new PR that removes the "exported by yamde" note entirely. I'd be OK with accepting it. (I always expect people will delete the note after they export anyhow.)
But could you update the PR to remove this comment and the whitespace change below, please?
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.
Oops made that while reading the code and commited it by accident :) (it was an ironic comment from me to myself - sorry about that)
Sure I will update it all.
|
Tried to rewrite history, but am too tired for this right now so just added a new commit. |
|
I'd like to merge this, but as-written it will fail if you don't have the patched simplemediawiki library (as there's no parameter named Can you please change the code so it still works with an unpatched simplemediawiki? (either using an if statement to choose which constructor to call,, or you can use the python **kwargs feature.) Please also document the new argument in the README with a link to the patched simplemediawiki versions. |
|
I mentioned the domain functionality in the README, but I won't link to a patched version or add a test for it right now since the domain support will be merged soon. Right now there are 2 different domain implementations which work kind of the same, but aren't so waiting for the official merge will be the better solution. |
mediawiki.py
Outdated
| def __init__(self, api_url, http_user=None, http_pass="", wiki_user=None, wiki_pass="", wiki_domain="", verbose=False): | ||
| self.verbose = verbose | ||
| self.mw = simplemediawiki.MediaWiki(api_url,http_user=http_user,http_password=http_pass) | ||
| self.mw = simplemediawiki.MediaWiki(api_url, http_user=http_user, http_password=http_pass, domain=wiki_domain) |
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.
I still can't merge this as-is because this line will crash if the user has an unpatched mediawiki install (the domain= argument won't exist.)
Can you change the code so it only passes domain= if that argument is set?
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.
Oops yeah sorry about that. Thought you were referring to it failing when the simplemediawiki lib didn't have the domain capability. I'm fixing this up right now and try to make it actually pullable.
Sorry that I'm causing this extra work for you.
|
Here I think this should suffice. I know check if the module is patched and have negated the crash through an if statement. If there is a more elegant way to handle this let me know. |
|
Seems good, thanks. |
You sometimes have to login through a domain. The domain functionality isn't provided by the default simplemediawiki, but there are forks and pull requests to add this (simple) functionality. This pull requests doesn't have checks if the domain functionality exists, but adds the option and a note.
A pull from one of the other sources or your own fork + an udate of the README should be sufficient.
Oh and the api doesn't allow pulls of the whole wiki right now and limits it to 10 entries by default and can be set to a max of 500 which this patch also fixes (I can split it if necessary).