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

'/' was resulting in '//' in change_url #368

Merged
merged 1 commit into from Sep 15, 2017

Conversation

Psycojoker
Copy link
Member

@Psycojoker Psycojoker commented Sep 7, 2017

Fix https://dev.yunohost.org/issues/999

Tested using "yunohost tools shell" and works as expected.

This seems to be quite blocking for the integration of change_url into apps according to feedback.

To test using "yunohost tools shell" do something like this:

$ yunohost tools shell
In [1]: from yunohost.app import normalize_url_path

In [2]: normalize_url_path("/")
Out[2]: '/'

In [3]: normalize_url_path("//")
Out[3]: '/'

In [4]: normalize_url_path("/////")
Out[4]: '/'

In [5]: normalize_url_path("a")                                                                                                                                                                                 
Out[5]: '/a/'

In [6]: normalize_url_path("qsd/qsd/qsd/")                                                                                                                                                                      
Out[6]: '/qsd/qsd/qsd/'

@@ -2105,3 +2106,10 @@ def random_password(length=8):

char_set = string.ascii_uppercase + string.digits + string.ascii_lowercase
return ''.join([random.SystemRandom().choice(char_set) for x in range(length)])


def normalize_url_path(url_path):
Copy link
Contributor

Choose a reason for hiding this comment

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

probably nitpicking, but this helper could be in a file within the https://github.com/YunoHost/yunohost/tree/unstable/src/yunohost/utils folder ? (maybe out of scope of this PR)

Copy link
Member Author

Choose a reason for hiding this comment

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

Probably. It would be ever greater in moulinette utils I think but that's mostly a "quick fix" PR to unblock the app group situation. I don't mind if we do those kind of modifications (as long as it don't block this PR for the next release) but I'm not super available for that now :x

Copy link
Member

Choose a reason for hiding this comment

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

Like this one ? 😉

# Normalize the url path syntax
# Handle the slash at the beginning of path and its absence at ending
# Return a normalized url path
#
# example: url_path=$(ynh_normalize_url_path $url_path)
# ynh_normalize_url_path example -> /example
# ynh_normalize_url_path /example -> /example
# ynh_normalize_url_path /example/ -> /example
# ynh_normalize_url_path / -> /
#
# usage: ynh_normalize_url_path path_to_normalize
# | arg: url_path_to_normalize - URL path to normalize before using it
ynh_normalize_url_path () {
path_url=$1
test -n "$path_url" || ynh_die "ynh_normalize_url_path expect a URL path as first argument and received nothing."
if [ "${path_url:0:1}" != "/" ]; then # If the first character is not a /
path_url="/$path_url" # Add / at begin of path variable
fi
if [ "${path_url:${#path_url}-1}" == "/" ] && [ ${#path_url} -gt 1 ]; then # If the last character is a / and that not the only character.
path_url="${path_url:0:${#path_url}-1}" # Delete the last character
fi
echo $path_url
}

Copy link
Member Author

Choose a reason for hiding this comment

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

That doesn't great to call that from python :x

Copy link
Member

Choose a reason for hiding this comment

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

Ah nvm I was thinking @opi wanted to make this available for apps

Copy link
Contributor

Choose a reason for hiding this comment

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

no, but not scoped in app.py ; But let's merge it now, and we'll cleanup later.

Copy link
Member

@JimboJoe JimboJoe left a comment

Choose a reason for hiding this comment

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

LGTM and tested OK.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants