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

Simple framework to port scripts into legacy #807

Merged
merged 14 commits into from
Nov 16, 2023

Conversation

AlexVCaron
Copy link
Collaborator

@AlexVCaron AlexVCaron commented Nov 13, 2023

Legacy scripts are added to the setup directly in version.py. We have to decide how long the use of a deprecated script remains correct. Since it is interface breaknig for our user, I would remove them when the next major version comes out.

To port a script into legacy :

  1. Move the script into scripts/legacy
  2. from scilpy.io.deprecator import deprecate_script
  3. Create a deprecation message : DEPRECATION_MSG = "..."
  4. Decorate the main :
    @deprecate_with_version(DEPRECATION_MSG, "<version when deprecation starts>")

For now, build will succeed even if there is outdated legacy scripts in the repository. However, they won't be executable (will raise an error by default). I will investigate in setuptools to find a way to do it.

@arnaudbore arnaudbore self-requested a review November 13, 2023 16:21
@arnaudbore
Copy link
Contributor

@pep8speaks
Copy link

pep8speaks commented Nov 13, 2023

Hello @AlexVCaron, Thank you for updating !

There are currently no PEP 8 issues detected in this Pull Request. Cheers! 🍻

Comment last updated at 2023-11-16 18:27:40 UTC

@arnaudbore
Copy link
Contributor

@arnaudbore
Copy link
Contributor

Build passed ! Good Job 🍻 !

@arnaudbore
Copy link
Contributor

@arnaudbore
Copy link
Contributor

@arnaudbore
Copy link
Contributor

@arnaudbore
Copy link
Contributor

@arnaudbore
Copy link
Contributor

Build passed ! Good Job 🍻 !

@EmmaRenauld
Copy link
Contributor

Tested on an upcomping branch on mine to deprecate the scil_streamline_math (already deprecated, but without the decorator).

(P.S. the description of this PR should read: the decorator of the main is @deprecate_script)

Works! Questions:

  • Here is the result:
    • It says dipy error. Ok?
    • It says: "as of version 2.0.0" although I used 1.7.0.
    • Anyway to hide the sys.exit()?
    • The warning only appears at the top of the help. So if users doesn't go back up (ex, after printing the whole help), he won't see it. ok for us?

deprecated

@arnaudbore
Copy link
Contributor

I think it would make more sense if @AlexVCaron you put scil_streamline_math script example in this PR.

@EmmaRenauld
Copy link
Contributor

@AlexVCaron
Copy link
Collaborator Author

I added the script. Do we keep a long expiration window on the version major ? Or we shorten it by targeting 2 or 3 minor revisions ahead ?

@arnaudbore
Copy link
Contributor

Build passed ! Good Job 🍻 !

@arnaudbore
Copy link
Contributor

Build passed ! Good Job 🍻 !

@arnaudbore
Copy link
Contributor

Build passed ! Good Job 🍻 !

@arnaudbore
Copy link
Contributor

Build passed ! Good Job 🍻 !

@arnaudbore
Copy link
Contributor

Build passed ! Good Job 🍻 !

1 similar comment
@arnaudbore
Copy link
Contributor

Build passed ! Good Job 🍻 !

@arnaudbore arnaudbore merged commit 0162e2d into scilus:master Nov 16, 2023
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.

4 participants