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] ynh_url_join #645

Closed
wants to merge 1 commit into
base: stretch-unstable
from

Conversation

Projects
None yet
4 participants
@kay0u
Copy link
Contributor

kay0u commented Feb 9, 2019

The problem

...

Solution

Add a helper that can join several url together

PR Status

Tested

How to test

ynh_url_join example admin index.php
ynh_url_join /example admin /index.php
ynh_url_join /

Validation

  • Principle agreement 0/2 :
  • Quick review 0/1 :
  • Simple test 0/1 :
  • Deep review 0/1 :
@maniackcrudelis

This comment has been minimized.

Copy link
Contributor

maniackcrudelis commented Feb 10, 2019

What's the purpose of this helper @kay0u ?

@kay0u

This comment has been minimized.

Copy link
Contributor Author

kay0u commented Feb 11, 2019

I needed it in an app, so I make a proposal...

it normalizes a URL from for example "path_url", and another variable.

@maniackcrudelis

This comment has been minimized.

Copy link
Contributor

maniackcrudelis commented Feb 11, 2019

Yep, so far I was guessing that.
My question was more what the use case of this helper?
Can't see any usage of it

@maniackcrudelis

This comment has been minimized.

Copy link
Contributor

maniackcrudelis commented Feb 11, 2019

But! Why not simply concatenate the path together!?
If it's only because of a double path, remove it. It's would be easier

@kay0u

This comment has been minimized.

Copy link
Contributor Author

kay0u commented Feb 11, 2019

That's exactly what i've done before, but i was not happy with an if path_url == "/"

But, if you think it's not a helper really helpfull, you can close this PR without any problem :-)

@maniackcrudelis

This comment has been minimized.

Copy link
Contributor

maniackcrudelis commented Feb 11, 2019

For me it looks like an edge case. Never heard before of an app for which it was really a problem.
Meanwhile, you can add this helper to the experimental repo and use it in dotclear.
While in experimental helpers repo, maybe it will meet its public.

Maybe other from @YunoHost/apps group would have a different opinion ?

@JimboJoe

This comment has been minimized.

Copy link
Member

JimboJoe commented Feb 11, 2019

Well I agree with @maniackcrudelis that you could submit it to experimental helpers first. Although it's ugly, double slashes have usually no impact... 🤷‍♂️

@kay0u kay0u closed this Feb 17, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment