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

Support special characters in wiki page titles. #222

Merged
merged 4 commits into from
Apr 25, 2019

Conversation

rczajka
Copy link
Contributor

@rczajka rczajka commented Mar 20, 2019

Redmine wiki pages titles can contain characters which need to be URL-encoded. This means the titles can't be pasted verbatim into the URL templates.

This change introduces a string.Formatter subclass which urrlib.parse.quotes the argument, and uses that to format the URLs where wiki page titles are used.

@coveralls
Copy link

coveralls commented Mar 20, 2019

Coverage Status

Coverage increased (+0.02%) to 98.607% when pulling 834a46a on rczajka:master into 137ea84 on maxtepkeev:master.

@maxtepkeev
Copy link
Owner

Awesome, looks fantastic, but I would like to have a closer look before merging, will try to do this ASAP.

@maxtepkeev
Copy link
Owner

Finally had some time to take a look, I really like the idea, but having thought about it a bit more I suggest we go a bit further, have a look at the following:

class ResourceQueryFormatter(string.Formatter):
    """
    Quotes query and memorizes all arguments, used during string formatting.
    """
    def __init__(self):
        self.used_kwargs = {}
        self.unused_kwargs = {}

    def check_unused_args(self, used_args, args, kwargs):
        self.used_kwargs = {item: kwargs.pop(item) for item in used_args if item in kwargs}
        self.unused_kwargs = kwargs

    def format_field(self, value, format_spec):
        return quote(super(ResourceQueryFormatter, self).format_field(value, format_spec).encode('utf-8'))


class ResourceQueryStr(str):
    formatter = ResourceQueryFormatter()

    def format(self, *args, **kwargs):
        return self.formatter.format(self, *args, **kwargs)

We can wrap all query_* strings in the Resource class with ResourceQueryStr using the Registrar metaclass and get rid of the direct calls to the custom string formatters in the code.

Also it should be pretty safe to just quote all the query_* attributes for all the resources and not just WikiPage.

@rczajka Thoughts ?

@rczajka
Copy link
Contributor Author

rczajka commented Apr 12, 2019

Looks like a good idea. One more thing I would probably do, just to keep the templating consistent, is then to clearly separate formatting the URLs from plain string manipulation, so change the remaining things like:

    @property
    def url(self):
        return '{0}/projects/{1}/issues?query_id={2}'.format(self.manager.redmine.url, …)

to things like:

    query_xyz = '/projects/{0}/issues?query_id={1}'

    @property
    def url(self):
        return ''.join((self.manager.redmine.url, self.query_xyz.format(…)

But as long as it doesn't cough up on weird characters, that's fine by me. :)

@maxtepkeev
Copy link
Owner

@rczajka Makes total sense. I think the url example is the only one that's not in the query_* family, so we can create a query_url and move it there.

Do you have time to make the necessary changes ?

If not that's also fine, we can merge this PR as is and I'll adjust the code a bit later. Just please remove the .gitignore file as it's not supposed to be there.

Thanks.

@rczajka
Copy link
Contributor Author

rczajka commented Apr 12, 2019

Sure, I should have same time for that in the next week.

@maxtepkeev
Copy link
Owner

Fantastic. Thanks!

@rczajka
Copy link
Contributor Author

rczajka commented Apr 25, 2019

Done.
Haven't touched query strings outside Resource subclasses (specifically, in Watcher and User).

Copy link
Owner

@maxtepkeev maxtepkeev left a comment

Choose a reason for hiding this comment

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

Beautiful, left a few comments, cosmetics mostly. We can merge afterwards.

redminelib/resources/base.py Outdated Show resolved Hide resolved
redminelib/resources/base.py Outdated Show resolved Hide resolved
redminelib/utilities.py Show resolved Hide resolved
redminelib/utilities.py Outdated Show resolved Hide resolved
@maxtepkeev maxtepkeev merged commit 5769a92 into maxtepkeev:master Apr 25, 2019
@maxtepkeev
Copy link
Owner

Fantastic. Thanks!

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.

3 participants