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

[mod] Remove deprecated helpers while still supporting them through hacky patching #780

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

Conversation

@alexAubin
Copy link
Member

commented Aug 17, 2019

The problem

Stuff like yunohost app initdb has been deprecated for more than 3 years now .. And I hate having legacy code all over the place with dozens of outdated strings :|

Solution

Remove those unused / deprecated helpers ... well actually I implemented some sort of auto-patching to try to keep the compatibility ... It can be tested using the apps unit tests which runs on the legacy test app

Should be noted that 90% of the apps that I found still using those helpers seem to be really outdated / notworking anyway ...

PR Status

Tested and working on my machine

How to test

Try to install some very outdated app still using stuff like yunohost app initdb or yunohost app checkport

Validation

  • Principle agreement 0/2 :
  • Quick review 0/1 :
  • Simple test 0/1 :
  • Deep review 0/1 :
@zamentur
Copy link
Contributor

left a comment

Quick reviewed: LGTM

content = pattern.sub(replace, content)
replaced_stuff = True
except Exception as e:
import pdb; pdb.set_trace()

This comment has been minimized.

Copy link
@Psycojoker

Psycojoker Aug 25, 2019

Member

Oupsi oupsi, debug code 😅 (btw "ipdb" or "pdbpp" are way better than pdb)

if helper in content and pattern != "":
try:
content = pattern.sub(replace, content)
replaced_stuff = True

This comment has been minimized.

Copy link
@Psycojoker

Psycojoker Aug 25, 2019

Member

Misc: that one would be better in an "else:" statement of the try/except but that's really minor.

import pdb; pdb.set_trace()

# If we couldn't patch the deprecated helper, abort the install or whichever step is performed
if helper in content:

This comment has been minimized.

Copy link
@Psycojoker

Psycojoker Aug 25, 2019

Member

Shouldn't that be an elif here 🤔 ? My understanding is that since it's an else and there is no return/continue previously it's going to be always matched.

This comment has been minimized.

Copy link
@alexAubin

alexAubin Aug 25, 2019

Author Member

Hmyea sorry I should maybe improve the comment here but basically :

  • if we detect that the helper is used, we attempt to patch it in the previous if
  • after this, if the helper is still in content, that mean we couldn't effectively patch it, e.g. because the actual usage did not match the format defined by the regex or whatever

@alexAubin alexAubin added this to the 3.8.x milestone Sep 1, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.