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

Implement deprecated for obsoleted options #1791

Merged
merged 1 commit into from
May 5, 2023
Merged

Conversation

psss
Copy link
Collaborator

@psss psss commented Jan 16, 2023

Add a new class Deprecated and a new parameter deprecated to tmt.utils.field() to allow marking obsolete options with the version since when it should not be used and an optional hint with the recommended alternative.

For example the following field:

tmt.utils.field(
    option="--old-option"
    deprecated=Deprecated(
        since="1.23",
        hint="Use --something-else instead."),
    help="I'm doing this and that.")

will produce this help message:

--old-option   I'm doing this and that. The option is
               deprecated since 1.23. Use --something-else
               instead.

Also document the multiple parameter in the field() docstring.

@psss psss self-assigned this Jan 16, 2023
@psss psss added this to the 1.21 milestone Jan 16, 2023
@psss
Copy link
Collaborator Author

psss commented Jan 16, 2023

/packit test

@psss
Copy link
Collaborator Author

psss commented Jan 17, 2023

Hm, so it seems there is no hidden option in click.Option on el8 as there's only old python3-click-6.7-8 available. So, I guess we drop this? Or, hide the option if hidden is available? @happz, what do you think?

@happz
Copy link
Collaborator

happz commented Jan 17, 2023

Hm, so it seems there is no hidden option in click.Option on el8 as there's only old python3-click-6.7-8 available. So, I guess we drop this? Or, hide the option if hidden is available? @happz, what do you think?

I think there's already some old-click-only magic somewhere already...

Anyway, how about not calling it "hidden" but rather "deprecated"? Then we'd express the goal in a declarative manner, marking the option as deprecated, leaving the hands of follow-up tools free to act differently. E.g. we could pass hidden=True to click.option() when running with recent-enough Click and add e.g. deprecated to the option's help before handing it to click.option(), and tmt could generate a nice warning when being given such an option, and so on. The important bit the field is deprecated/obsolete, hidden=True is just one of the many possible reactions.

Maybe it could be a non-boolean option, maybe a string to carry tmt version, as in "deprecated since x.y"?

A bit too verbose, but I included the answer to your question: with old Click, I'd add "deprecated" to the field help, with new Click I'd do that & add hidden=True as well. But it depends on the train of thoughts ^^

@psss
Copy link
Collaborator Author

psss commented Jan 17, 2023

Using deprecated sounds even better! Let's handle it in this way. Regarding the type: Not sure how much it is interesting for the user to know since when the option is deprecated... On the other hand, it could be quite useful to know when the option will be removed (if it will be). Not sure. Both bool and str are fine with me. @lukaszachy, @adiosnb, thoughts?

@happz
Copy link
Collaborator

happz commented Jan 17, 2023

I for one would prefer the string, deprecated='1.21', for example. It can always be ignored, it's bool-ish as any other string, and the info can be used by tmt itself, for example: when such a field is initialized from fmf or CLI option, tmt could emit a warning. I didn't think about such a possibility before, but I like it more and more, especially with some fields waiting to be changed (e.g. tmt * export options being moved to their respective plugins - main command could retaint them, for backward compatibility, and hint user automagically, without plugins taking care of that). It would also go with current schema validation warnings, and tmt lint could print out "you're using a key deprecated in 1.21, it's 1.956 now, maybe it's time to update" messages.

Is it enough? :)

@psss
Copy link
Collaborator Author

psss commented Jan 17, 2023

"you're using a key deprecated in 1.21, it's 1.956 now, maybe it's time to update"

You got me! :-)
+1 for the string.

@psss psss modified the milestones: 1.21, 1.22 Jan 31, 2023
@psss psss requested a review from thrix as a code owner April 4, 2023 14:31
@psss psss modified the milestones: 1.22, 1.23 Apr 5, 2023
@happz happz self-requested a review April 27, 2023 09:31
@psss psss changed the title Support hidden options in utils.field() Implement deprecated for obsoleted options May 4, 2023
Add a new class `Deprecated` and a new parameter `deprecated` to
`tmt.utils.field()` to allow marking obsolete options with the version
since when it should not be used and an optional hint with the
recommended alternative.

For example the following field:

    tmt.utils.field(
        option="--old-option"
        deprecated=Deprecated(
            since="1.23",
            hint="Use --something-else instead."),
        help="I'm doing this and that.")

will produce this help message:

    --old-option   I'm doing this and that. The option is
                   deprecated since 1.23. Use --something-else
                   instead.

Also document the `multiple` parameter in the `field()` docstring.
@happz happz merged commit ace7285 into main May 5, 2023
17 checks passed
@happz happz deleted the psss-hidden-options branch May 5, 2023 08:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants