Opened 5 years ago

Closed 2 years ago

Last modified 2 years ago

#4086 enhancement closed fixed (fixed)

tap2rpm has an unused "unsigned" command-line option.

Reported by: TimAllen Owned by:
Priority: normal Milestone:
Component: core Keywords: easy
Cc: thijs, rami.chowdhury@… Branch: branches/deprecate-tap2rpm-4086
(diff, github, buildbot, log)
Author: cyli Launchpad Bug:

Description (last modified by thijs)

twisted.scripts.tap2rpm.MyOptions defines a flag called "unsigned" that does not appear to be used anywhere. I believe RPMs are unsigned unless you pass --sign to rpmbuild, so a special flag seems unnecessary.

Attachments (29)

remove_unsigned.patch (396 bytes) - added by jtanis 5 years ago.
removed optFlags altogether, since unsigned is the only option.
remove_unsigned.2.patch (396 bytes) - added by jtanis 5 years ago.
removed optFlags altogether, since unsigned is the only option.
remove_unsigned.3.patch (396 bytes) - added by jtanis 5 years ago.
removed optFlags altogether, since unsigned is the only option.
remove_unsigned.4.patch (396 bytes) - added by jtanis 5 years ago.
removed optFlags altogether, since unsigned is the only option.
remove_unsigned.5.patch (396 bytes) - added by jtanis 5 years ago.
removed optFlags altogether, since unsigned is the only option.
remove_unsigned.6.patch (396 bytes) - added by jtanis 5 years ago.
removed optFlags altogether, since unsigned is the only option.
remove_unsigned.7.patch (396 bytes) - added by jtanis 5 years ago.
removed optFlags altogether, since unsigned is the only option.
remove_unsigned.8.patch (396 bytes) - added by jtanis 5 years ago.
removed optFlags altogether, since unsigned is the only option.
remove_unsigned.9.patch (396 bytes) - added by jtanis 5 years ago.
removed optFlags altogether, since unsigned is the only option.
remove_unsigned.10.patch (396 bytes) - added by jtanis 5 years ago.
removed optFlags altogether, since unsigned is the only option.
remove_unsigned.11.patch (396 bytes) - added by jtanis 5 years ago.
removed optFlags altogether, since unsigned is the only option.
remove_unsigned.12.patch (396 bytes) - added by jtanis 5 years ago.
removed optFlags altogether, since unsigned is the only option.
remove_unsigned.13.patch (396 bytes) - added by jtanis 5 years ago.
removed optFlags altogether, since unsigned is the only option.
remove_unsigned.14.patch (396 bytes) - added by jtanis 5 years ago.
removed optFlags altogether, since unsigned is the only option.
remove_unsigned.15.patch (396 bytes) - added by jtanis 5 years ago.
removed optFlags altogether, since unsigned is the only option.
remove_unsigned.16.patch (396 bytes) - added by jtanis 5 years ago.
removed optFlags altogether, since unsigned is the only option.
remove_unsigned.17.patch (396 bytes) - added by jtanis 5 years ago.
removed optFlags altogether, since unsigned is the only option.
remove_unsigned.18.patch (396 bytes) - added by jtanis 5 years ago.
removed optFlags altogether, since unsigned is the only option.
remove_unsigned.19.patch (396 bytes) - added by jtanis 5 years ago.
removed optFlags altogether, since unsigned is the only option.
remove_unsigned.20.patch (396 bytes) - added by jtanis 5 years ago.
removed optFlags altogether, since unsigned is the only option.
remove_unsigned.22.patch (396 bytes) - added by jtanis 5 years ago.
removed optFlags altogether, since unsigned is the only option.
remove_unsigned.21.patch (396 bytes) - added by jtanis 5 years ago.
removed optFlags altogether, since unsigned is the only option.
remove_unsigned.23.patch (396 bytes) - added by jtanis 5 years ago.
removed optFlags altogether, since unsigned is the only option.
remove_unsigned.24.patch (396 bytes) - added by jtanis 5 years ago.
removed optFlags altogether, since unsigned is the only option.
twisted4086.patch (2.2 KB) - added by necaris 2 years ago.
Patch to deprecate "unsigned" flag, with test
twisted4086.2.patch (2.8 KB) - added by necaris 2 years ago.
Patch to deprecate "unsigned" flag, with test and news file
twisted4086.3.patch (5.1 KB) - added by necaris 2 years ago.
Patch to deprecate "unsigned" flag, with test and news file, maintaining short-form option
twisted4086.4.patch (5.1 KB) - added by necaris 2 years ago.
Patch to deprecate "unsigned" flag, with test and news file, corrected deprecating version
twisted4086.5.patch (4.8 KB) - added by necaris 2 years ago.
Patch to deprecate "unsigned" flag, now with better docstring

Download all attachments as: .zip

Change History (47)

comment:1 Changed 5 years ago by TimAllen

This ticket split off from #3292.

Changed 5 years ago by jtanis

removed optFlags altogether, since unsigned is the only option.

Changed 5 years ago by jtanis

removed optFlags altogether, since unsigned is the only option.

Changed 5 years ago by jtanis

removed optFlags altogether, since unsigned is the only option.

Changed 5 years ago by jtanis

removed optFlags altogether, since unsigned is the only option.

Changed 5 years ago by jtanis

removed optFlags altogether, since unsigned is the only option.

Changed 5 years ago by jtanis

removed optFlags altogether, since unsigned is the only option.

Changed 5 years ago by jtanis

removed optFlags altogether, since unsigned is the only option.

Changed 5 years ago by jtanis

removed optFlags altogether, since unsigned is the only option.

Changed 5 years ago by jtanis

removed optFlags altogether, since unsigned is the only option.

Changed 5 years ago by jtanis

removed optFlags altogether, since unsigned is the only option.

Changed 5 years ago by jtanis

removed optFlags altogether, since unsigned is the only option.

Changed 5 years ago by jtanis

removed optFlags altogether, since unsigned is the only option.

Changed 5 years ago by jtanis

removed optFlags altogether, since unsigned is the only option.

Changed 5 years ago by jtanis

removed optFlags altogether, since unsigned is the only option.

Changed 5 years ago by jtanis

removed optFlags altogether, since unsigned is the only option.

Changed 5 years ago by jtanis

removed optFlags altogether, since unsigned is the only option.

Changed 5 years ago by jtanis

removed optFlags altogether, since unsigned is the only option.

Changed 5 years ago by jtanis

removed optFlags altogether, since unsigned is the only option.

Changed 5 years ago by jtanis

removed optFlags altogether, since unsigned is the only option.

Changed 5 years ago by jtanis

removed optFlags altogether, since unsigned is the only option.

comment:2 Changed 5 years ago by jtanis

  • Keywords review added
  • Resolution set to fixed
  • Status changed from new to closed

Marked for review, sorry about the spam up there.

comment:3 Changed 5 years ago by jtanis

  • Resolution fixed deleted
  • Status changed from closed to reopened

comment:4 Changed 5 years ago by jtanis

  • Owner changed from glyph to jtanis
  • Status changed from reopened to new

comment:5 Changed 5 years ago by jtanis

  • Owner jtanis deleted

comment:6 Changed 5 years ago by terrycojones

This is going to need a little (easy) work.

  1. The flag shouldn't be removed, instead its use should be deprecated. A ticket can later be opened to remove the flag.
  2. You should add a test/test_tap2rpm.py that imports the MyOptions class and calls parseOptions to make sure the deprecation warning is raised. Take a look in twisted/test/*.py for testing of deprecation warnings. You only need write the single test for the change you're making.

comment:7 Changed 5 years ago by terrycojones

  • Keywords review removed

comment:8 Changed 5 years ago by exarkun

  • Owner set to jtanis

comment:9 Changed 4 years ago by Screwtape

Note that #3292 has landed now, and made various changes to the MyOptions class. The "unsigned" command-line option is still there, however, so this ticket still needs fixing.

comment:10 Changed 4 years ago by thijs

  • Cc thijs added

Changed 2 years ago by necaris

Patch to deprecate "unsigned" flag, with test

comment:11 Changed 2 years ago by necaris

  • Cc rami.chowdhury@… added
  • Keywords review added
  • Owner jtanis deleted

comment:12 Changed 2 years ago by thijs

  • Keywords review removed
  • Owner set to necaris

Thanks for your patch. Can you include the Twisted version number (12.1) in the deprecation message as you see in other deprecations? It also needs a .removal NEWS file.

Changed 2 years ago by necaris

Patch to deprecate "unsigned" flag, with test and news file

Changed 2 years ago by necaris

Patch to deprecate "unsigned" flag, with test and news file, maintaining short-form option

comment:13 Changed 2 years ago by necaris

  • Keywords review added
  • Owner necaris deleted

Thanks for the review. I've changed the way the option is handled, so that the twisted.python.deprecate machinery can be used to generate a more standard deprecation warning, and added a NEWS file as requested.

comment:14 Changed 2 years ago by thijs

  • Description modified (diff)
  • Keywords review removed
  • Owner set to necaris

Thanks, the warning references 12.2 instead of 12.1 though.

Changed 2 years ago by necaris

Patch to deprecate "unsigned" flag, with test and news file, corrected deprecating version

comment:15 Changed 2 years ago by necaris

  • Keywords review added
  • Owner necaris deleted

Oops, my mistake -- corrected patch.

comment:16 Changed 2 years ago by cyli

  • Keywords review removed
  • Owner set to necaris

Thank you for all your hard work necaris! And thanks so much for replacing all those tabs!

The docstring for opt_XXX in usage.Options subclasses are actually presented to the user whe they ask for usage help. For example, if you look at the opt_XXXX docstrings in twisted.web.tap.py, and then run twistd web --help, you'll see that they match.

Can you please change the docstring to it's more user-help friendly? For example, the "Handle the 'unsigned' command line flag" is more like a docstring aimed at developers.

Also, this is a minor nitpick, but there should be two lines between class methods (opt_unsigned has only 1 line between it and the previous method). (for reference, here is the part of the coding standard dealing with whitespace).

Once again, thank you so much for working on this!

Changed 2 years ago by necaris

Patch to deprecate "unsigned" flag, now with better docstring

comment:17 Changed 2 years ago by necaris

  • Keywords review added
  • Owner necaris deleted

Thank you for your reviews cyli! I've updated the patch, with the right convention regarding spacing and a more helpful docstring (modeled on a deprecated option to twisted.tap.ftp -- hope that was a good one to follow).

comment:18 Changed 2 years ago by cyli

  • Author set to cyli
  • Branch set to branches/deprecate-tap2rpm-4086

(In [33743]) Branching to 'deprecate-tap2rpm-4086'

comment:19 Changed 2 years ago by cyli

(In [33744]) Apply patch from necaris (with newsfile from previous patch)

refs #4086

comment:20 Changed 2 years ago by cyli

  • Keywords review removed

Thanks for your prompt responses necaris! This looks great (buildbot results).

Going to merge into trunk.

comment:21 Changed 2 years ago by cyli

  • Resolution set to fixed
  • Status changed from new to closed

(In [33747]) Merge deprecate-tap2rpm-4086: deprecate the --unsigned option in tap2rpm

Author: necaris
Reviewer: thijs, cyli
Fixes: #4086

The --unsigned option in tap2rpm is now deprecated

comment:22 Changed 2 years ago by necaris

Thank you guys for your patient reviews while I fumbled my way towards a usable patch!

Note: See TracTickets for help on using tickets.