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

Add append/prepend setting in save & use this to add touch #232

Merged
merged 5 commits into from Oct 10, 2019

Conversation

RheingoldRiver
Copy link
Member

Add support for using appendtext or prependtext instead of text in an api edit request. This also allows a graceful way to blank/null edit a page without having to spend two api queries to get the text and then save, so add page.touch() as well using this.

Copy link
Member

@danmichaelo danmichaelo left a comment

Choose a reason for hiding this comment

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

Sorry it took so long to get back to you on this! We should definitely add support for append/prepend, but perhaps it's better to introduce new methods to keep the complexity of the already quite complex save method down. If we add the following methods, there should be no need to modify the save method or add the new exception:

    def append(self, text, summary=u'', minor=False, bot=True, **kwargs):
        """Append text to a page by performing an edit operation."""
        return self.save(
            text=None, summary=summary, minor=minor, bot=bot, appendtext=text, **kwargs
        )

    def prepend(self, text, summary=u'', minor=False, bot=True, **kwargs):
        """Prepend text to a page by performing an edit operation."""
        return self.save(
            text=None, summary=summary, minor=minor, bot=bot, prependtext=text, **kwargs
        )

What do you think?

When it comes to the touch method, it would be good if the docstring explained how it differs from the purge method.

mwclient/page.py Outdated
**data)
result = None
if edit_type == 'replace':
result = self.site.post('edit', title=self.name, text=text,
Copy link
Member

Choose a reason for hiding this comment

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

Hm, it's not so elegant to repeat this code three times. I wonder if it would be better to add two new methods append(text) and prepend(text) that calls save(...)

Copy link
Member Author

Choose a reason for hiding this comment

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

How about this:

    def save(self, text, summary=u'', minor=False, bot=True, section=None, **kwargs):
        """Update the text of a section or the whole page by performing an edit operation.
                """
        return self._save(summary, minor, bot, section, text=text, **kwargs)
    
    def save_append(self, text, summary=u'', minor=False, bot=True, section=None,
                    **kwargs):
        """Append text to the text of a section or the whole page by performing an edit operation.
                """
        return self._save(summary, minor, bot, section, append_text=text, **kwargs)
    
    def save_prepend(self, text, summary=u'', minor=False, bot=True, section=None,
                     **kwargs):
        """Prepend text to the text of a section or the whole page by performing an edit operation.
                """
        return self._save(summary, minor, bot, section, prepend_text=text, **kwargs)

and then below in _save,

 def do_edit():
            result = self.site.post('edit', title=self.name, summary=summary,
                                    token=self.get_token('edit'),
                                        **data)

I didn't test yet but if that looks good to you I'll run tests, fix the docstring for touch, and send a new commit - sorry for the whitespace issues, I'll try to fix those too.

Copy link
Member

Choose a reason for hiding this comment

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

Did you see my other suggestion above? I think I would prefer having methods named just append and prepend rather than save_append and save_prepend. Do you agree?

Copy link
Member Author

Choose a reason for hiding this comment

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

Prepend/Append didn't sound obviously enough like page-saving actions to me out of context, and if they start with save_ then they'd show up in autocomplete along with save, is why I renamed them. But if you'd still prefer just prepend/append I'm fine with that just to have simpler names for everything.

Copy link
Member

Choose a reason for hiding this comment

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

I still find the save_ names a bit clumsy, but it sounds useful to have them show up in autocomplete along with save, yes, I agree. We could try getting a third opinion on this. @waldyrious, do you have a preference here?

Copy link
Member

Choose a reason for hiding this comment

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

Sure, no objections to keeping save() as an alias for compatibility.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ok great, I have a pretty bad cold right now but I'll send new changes by the end of the week:

  • edit, prepend, and append will call _edit
  • _edit will be similar to current save except text/append_text/prepend_text will be part of **kwargs
  • alias of save for edit
  • update touch to work with new method & add clarity in docstring

Copy link
Member

Choose a reason for hiding this comment

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

Wish you a quick recovery ✨

Sounds like a good plan. My first thought was that append and prepend could just call save edit(text=None, ...), but I'm open to the idea of adding a new _edit method if you think that's less messy :)

Copy link
Member Author

Choose a reason for hiding this comment

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

Ok made the changes. Do you have a settings file I can import to Pycharm so I have the right whitespace settings for everything?

Copy link
Member

Choose a reason for hiding this comment

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

Do you have a settings file I can import to Pycharm so I have the right whitespace settings for everything?

.editorconfig files are natively supported by PyCharm, so it should have picked up the definitions we have already. Maybe there are additional configurations we need to add to the file?

Copy link
Member

@danmichaelo danmichaelo left a comment

Choose a reason for hiding this comment

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

If we plan to eventually remove the save() method, we should also add a DeprecationWarning to that method. But since this is probably one of the more commonly used methods of this library, perhaps we should just keep it indefinitely as an alias to avoid breaking things? I think I'm lending towards that.

Docs should be updated to use the new method though. I think this is the only page that needs to be updated: https://github.com/mwclient/mwclient/blob/master/docs/source/user/page-ops.rst

And it would also be good to update the tests here to use the new edit() method: https://github.com/mwclient/mwclient/blob/0e0d85aac93de88b7eadfc99a8a37668f6b0a4e9/test/test_page.py

@RheingoldRiver
Copy link
Member Author

perhaps we should just keep it indefinitely as an alias to avoid breaking things? I think I'm lending towards that.

Agreed, I can't imagine wanting to use that name for something else, and just thinking about my own mwclient usage, that's so very many things I'd have to change.

Updated the tests and docs, and fixed the line that was 91 characters long.

@danmichaelo danmichaelo merged commit 52fb803 into mwclient:master Oct 10, 2019
@RheingoldRiver RheingoldRiver deleted the append-prepend branch October 10, 2019 15:24
@waldyrious
Copy link
Member

Thanks for the contribution and your patience, @RheingoldRiver! 🎉

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

3 participants