Add standardization - Githubissues

YunoHost-Apps / redirect_ynh

Redirection app for YunoHost

GNU Affero General Public License v3.0

43 stars 20 forks source link

Add standardization #11

Closed Jibec closed 5 years ago

Jibec commented 6 years ago

Problem

This apps looks like to need an update about how we do packages now. Here is some love. I'll open a new PR if this one is accepted.

Here is a proposal:

to license the code under AGPL3+
to use correct helpers so the package linter says SUCCESS

Solution

mostly minor fixes

I need you to confirm:

that the license fits you
that the private/public behavior match your expectation (package_check cries but my tests says it works..)

PR Status

[x] Code finished.
[x] Tested with Package_check.
[x] Fix or enhancement tested.
[x] Upgrade from last version tested.
[x] Can be reviewed and tested.

keomabrun commented 5 years ago

Yop! Any new on this PR ?

Jibec commented 5 years ago

hello, no, so far, I had no feedback from the maintainer of this app and stopped using it myself. I let it open so work isn't lost, please feel welcome to continue this work. I'll be pleased to answer your question.

arthurlutz commented 5 years ago

Before

screenshot from 2019-01-13 21-16-50

After

screenshot from 2019-01-13 21-17-07

Jibec commented 5 years ago

I personally don't use this app anymore. You should run real world tests before merging this. I'm ready to help if needed.

arthurlutz commented 5 years ago

Created a testing branch to test out these changes.

scith commented 5 years ago

Sorry, I have been out of the project for a while. Thanks a lot for these changes. They look good to me. I haven't tested them yet, though.

Jibec commented 5 years ago

@arthurlutz and @scith : I understand from https://github.com/YunoHost-Apps/redirect_ynh/issues/13 that you now are maintainer but the manifest says it's opi (no quote to prevent emailing him) So now I understand why you answered here :)

Can you please move this PR to the testing branch or tell me how to do it?

opi commented 5 years ago

Nice job @jibec ! I have a quick look at the code and it seems ok, I hope to have some time to test it. But I trust @arthurlutz & @scith , and we can create another PR to add them as maintainers. (btw, @Jibec , if you've split your work into multiple PR, review & merge may have been easier)

thanks everyone

Jibec commented 5 years ago

Yeah, I know, I tried multiple times to make a step by step refactoring of apps, but never successed at it... I always wondered how to do it properly.

alexAubin commented 5 years ago

Smol bump ... any news about this ? It looks like this app is kinda unmaintained ... We could just agree collectively to merge this PR which doesn't seem to introduce any big changes...

alexAubin commented 5 years ago

Soooo idk, proposing to merge this in a few days ...

alexAubin commented 5 years ago

Checked with https://ci-apps-dev.yunohost.org/jenkins/job/redirect_ynh%20(aleks)/1/console that there's no regression (well currently the app is level 0 in the app list apparently...) so merging