Remove the curses dialog, thereby deprecating the --help and --dialog command line options #3665

ohemorange · 2016-10-20T18:02:19Z

As per #3324.

… command line options

coveralls · 2016-10-20T18:14:49Z

Coverage decreased (-0.01%) to 98.759% when pulling 7d2b6ca on remove_dialog_two into b9adb7c on master.

ohemorange · 2016-10-20T18:18:59Z

Coverage went down because I removed covered lines, and also because I fixed some of the testing code that runs when the tests break.

jsha

Overall LGTM. I am excited for this change!

jsha · 2016-10-20T23:50:33Z

certbot/cli.py

@@ -677,7 +666,7 @@ def prepare_and_parse_args(plugins, args, detect_defaults=False):  # pylint: dis
        "e.g. -vvv.")
    helpful.add(
        None, "-t", "--text", dest="text_mode", action="store_true",
-        help="Use the text output instead of the curses UI.")
+        help="Use the text output.")


Shouldn't this also say "Deprecated - this has no effect?"

That it should!

Oh right now I remember, I left this in on the off chance we eventually implement something else for the IDialog interface. I'll make this clear though.

jsha · 2016-10-20T23:59:57Z

docs/contributing.rst

@@ -246,7 +246,7 @@ configuration checkpoints and rollback.
 Display
 ~~~~~~~

-We currently offer a pythondialog and "text" mode for displays. Display
+We currently offer a "text" mode for displays. Display


This should probably say "We currently only offer.."

bmw

I wanted to review this as well since this is such a dramatic change. For example, we have to be careful when modifying interfaces in certbot/interfaces.py as they may be used by our 3rd party plugins, however, as of writing this, there are apparently no known 3rd party plugins that use IDisplay.

Other than my two comments, this LGTM. I love how much code this PR is deleting.

bmw · 2016-10-21T00:42:37Z

docs/cli-help.txt

@@ -22,14 +22,11 @@ optional arguments:
  -v, --verbose         This flag can be used multiple times to incrementally
                        increase the verbosity of output, e.g. -vvv. (default:
                        -2)
-  -t, --text            Use the text output instead of the curses UI.


This is another of our "only modify during releases" files, except this one doesn't have a test to catch the problem. The reason we don't modify this is we use this file to generate our documentation at https://certbot.eff.org/docs/using.html#command-line and we want to keep this information synced with the current release.

Ok it seems the correct choice here is to not generate the documentation on that website based on master, but instead to generate it based on the release branch. Is there already an issue for that, or a reason why we don't already do that?

We often change the documentation for reasons other than a new release. We could delay publishing changes until a release has been done, but blocking all improvements to our documentation to keep this file in lockstep seems undesirable to me.

Are these documentation changes urgent enough that they can't wait for a new release? If we release regularly, this shouldn't be a problem.

They are rarely urgent, however, they do offer a better experience for the user. Why should we tie our hands and force ourselves to sit on these changes until we do a release?

And while I wouldn't call them urgent, there are definitely times we want to make changes outside of a release. For example, the stuff that I've been working on involves doc changes and needs to go live almost immediately so it is up before super secret project that I'm not sure I'm allowed to talk about publicly involving encryption, pipes, and Noah the hacker is released. Another example of changes like this is changes in OS packaging such as when a new version of a distro is released.

Ok, sounds like we want an update schedule for docs that is decoupled from the certbot release schedule. Let's put it in a new repo! I'm fine with the anti-pattern of "you can't change these files in master until release" if it's because we used to think that was ok and changing it now will break things, but for another site we control that seems strange.

Anyway, for now I'll move the docs changes into a separate PR and have that chill until release.

Yeah I think there are a number of things we could do to make this a little nicer for developers. I think it could get a bit tricky though.

And no need to make a separate PR. These changes happen automatically in our release script.

Cool! I also modified docs/contributing.rst, should I make a separate PR for that one?

Oh sorry. I missed that one. Yeah I suppose you should make a separate PR that we can merge during the release.

bmw · 2016-10-21T00:56:53Z

certbot/cli.py

@@ -686,7 +675,7 @@ def prepare_and_parse_args(plugins, args, detect_defaults=False):  # pylint: dis
              "which ones are required if it finds one missing")
    helpful.add(
        None, "--dialog", dest="dialog_mode", action="store_true",
-        help="Run using interactive dialog menus")
+        help="Deprecated - this has no effect")


I think we should suppress help output here rather than continuing to print messages about a deprecated flag. We have a utility function for deprecating arguments called add_deprecated_argument that does this. I think the only reason we may not want to use this is it will cause a warning to be printed whenever the flag is used. This might be overkill for flags like --text that might be used in renewal scripts.

If we don't want to print a warning, we should use argparse.SUPPRESS here as it's done in add_deprecated_argument.

I wonder if literally anyone ever used the --dialog flag, I have no problem deprecating that one properly.

As for --text, we have a whole bunch of tests that use it, I think suppressing it is the right move.

I agree. Let's deprecate --dialog and suppress help output from --text.

coveralls · 2016-10-21T01:15:34Z

Coverage decreased (-0.01%) to 98.759% when pulling 4eeec42 on remove_dialog_two into b9adb7c on master.

ohemorange · 2016-10-21T20:34:09Z

Re: certbot/interfaces.py - I'm fine keeping it in the interface and continuing to ignore it, on the off chance that this breaks some closed-source plugin.

bmw · 2016-10-21T21:19:58Z

I personally don't think reverting changes to certbot/interfaces.py and the notification methods in certbot/display/util.py is necessary. I was just bringing that up as an example of the kind of thing we need be careful about. I looked into it though and it doesn't seem to be a problem.

While it's possible there is some closed source plugin out there, none of us have ever heard about it. I personally think we should take advantage of this and use it as a chance to clean up the API a bit. I don't have a strong opinion here though, so feel free to revert these changes if you wish.

coveralls · 2016-10-21T21:21:25Z

Coverage decreased (-0.01%) to 98.76% when pulling f19eeda on remove_dialog_two into ce252bd on master.

coveralls · 2016-10-21T22:11:08Z

Coverage decreased (-0.01%) to 98.76% when pulling ced5f34 on remove_dialog_two into ce252bd on master.

coveralls · 2016-10-21T22:24:23Z

Coverage decreased (-0.01%) to 98.76% when pulling 25c8d18 on remove_dialog_two into ce252bd on master.

coveralls · 2016-10-21T22:30:13Z

Coverage decreased (-0.01%) to 98.76% when pulling 25c8d18 on remove_dialog_two into ce252bd on master.

ohemorange · 2016-10-21T22:31:02Z

All requested changes made!

Remove dialog, which is dropped since certbot/certbot#3665

Remove the curses dialog, thereby deprecating the --help and --dialog…

7d2b6ca

… command line options

ohemorange added this to the 0.10.0 milestone Oct 20, 2016

ohemorange assigned pde and bmw Oct 20, 2016

ohemorange mentioned this pull request Oct 20, 2016

Logger info messages go to stderr #3666

Open

jsha suggested changes Oct 21, 2016

View reviewed changes

documentation updates

4eeec42

bmw requested changes Oct 21, 2016

View reviewed changes

Merge branch 'master' into remove_dialog_two

f19eeda

Deprecate --dialog and suppress --text

ced5f34

ohemorange added 2 commits October 21, 2016 22:16

don't update docs/cli-help.txt before release

dbcbe80

don't modify docs/contributing.rst

25c8d18

jsha approved these changes Oct 21, 2016

View reviewed changes

bmw approved these changes Oct 21, 2016

View reviewed changes

bmw merged commit d54cb76 into master Oct 21, 2016

This was referenced Oct 26, 2016

Allow user to select all domains by typing empty string at checklist #3693

Merged

letsencrypt-auto fails with PythonDialogBug on Ubuntu 14.04.2 LTS #1154

Closed

pde deleted the remove_dialog_two branch November 18, 2016 20:27

pde removed the has pr label Dec 8, 2016

archlinux-github pushed a commit to archlinux/aur that referenced this pull request Jul 29, 2022

Bump to 0.11.0.r179.gb3116af5

f90061d

Remove dialog, which is dropped since certbot/certbot#3665

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove the curses dialog, thereby deprecating the --help and --dialog command line options #3665

Remove the curses dialog, thereby deprecating the --help and --dialog command line options #3665

ohemorange commented Oct 20, 2016

coveralls commented Oct 20, 2016

ohemorange commented Oct 20, 2016

jsha left a comment

jsha Oct 20, 2016

ohemorange Oct 21, 2016

ohemorange Oct 21, 2016

jsha Oct 20, 2016

ohemorange Oct 21, 2016

bmw left a comment

bmw Oct 21, 2016

ohemorange Oct 21, 2016

bmw Oct 21, 2016

ohemorange Oct 21, 2016

bmw Oct 21, 2016

ohemorange Oct 21, 2016

bmw Oct 21, 2016

ohemorange Oct 21, 2016

bmw Oct 21, 2016

bmw Oct 21, 2016

ohemorange Oct 21, 2016

bmw Oct 21, 2016

coveralls commented Oct 21, 2016

ohemorange commented Oct 21, 2016

bmw commented Oct 21, 2016

coveralls commented Oct 21, 2016

coveralls commented Oct 21, 2016

coveralls commented Oct 21, 2016

coveralls commented Oct 21, 2016

ohemorange commented Oct 21, 2016

Remove the curses dialog, thereby deprecating the --help and --dialog command line options #3665

Remove the curses dialog, thereby deprecating the --help and --dialog command line options #3665

Conversation

ohemorange commented Oct 20, 2016

coveralls commented Oct 20, 2016

ohemorange commented Oct 20, 2016

jsha left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bmw left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Oct 21, 2016

ohemorange commented Oct 21, 2016

bmw commented Oct 21, 2016

coveralls commented Oct 21, 2016

coveralls commented Oct 21, 2016

coveralls commented Oct 21, 2016

coveralls commented Oct 21, 2016

ohemorange commented Oct 21, 2016