Ticket #4086 enhancement closed fixed

Opened 4 years ago

Last modified 15 months ago

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
Author: cyli Launchpad Bug:

Description (last modified by thijs) (diff)

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

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

Change History

1

Changed 4 years ago by TimAllen

This ticket split off from #3292.

Changed 3 years ago by jtanis

removed optFlags altogether, since unsigned is the only option.

Changed 3 years ago by jtanis

removed optFlags altogether, since unsigned is the only option.

Changed 3 years ago by jtanis

removed optFlags altogether, since unsigned is the only option.

Changed 3 years ago by jtanis

removed optFlags altogether, since unsigned is the only option.

Changed 3 years ago by jtanis

removed optFlags altogether, since unsigned is the only option.

Changed 3 years ago by jtanis

removed optFlags altogether, since unsigned is the only option.

Changed 3 years ago by jtanis

removed optFlags altogether, since unsigned is the only option.

Changed 3 years ago by jtanis

removed optFlags altogether, since unsigned is the only option.

Changed 3 years ago by jtanis

removed optFlags altogether, since unsigned is the only option.

Changed 3 years ago by jtanis

removed optFlags altogether, since unsigned is the only option.

Changed 3 years ago by jtanis

removed optFlags altogether, since unsigned is the only option.

Changed 3 years ago by jtanis

removed optFlags altogether, since unsigned is the only option.

Changed 3 years ago by jtanis

removed optFlags altogether, since unsigned is the only option.

Changed 3 years ago by jtanis

removed optFlags altogether, since unsigned is the only option.

Changed 3 years ago by jtanis

removed optFlags altogether, since unsigned is the only option.

Changed 3 years ago by jtanis

removed optFlags altogether, since unsigned is the only option.

Changed 3 years ago by jtanis

removed optFlags altogether, since unsigned is the only option.

Changed 3 years ago by jtanis

removed optFlags altogether, since unsigned is the only option.

Changed 3 years ago by jtanis

removed optFlags altogether, since unsigned is the only option.

Changed 3 years ago by jtanis

removed optFlags altogether, since unsigned is the only option.

2

Changed 3 years ago by jtanis

  • status changed from new to closed
  • keywords review added
  • resolution set to fixed

Marked for review, sorry about the spam up there.

3

Changed 3 years ago by jtanis

  • status changed from closed to reopened
  • resolution fixed deleted

4

Changed 3 years ago by jtanis

  • owner changed from glyph to jtanis
  • status changed from reopened to new

5

Changed 3 years ago by jtanis

  • owner jtanis deleted

6

Changed 3 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.

7

Changed 3 years ago by terrycojones

  • keywords review removed

8

Changed 3 years ago by exarkun

  • owner set to jtanis

9

Changed 3 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.

10

Changed 3 years ago by thijs

  • cc thijs added

Changed 15 months ago by necaris

Patch to deprecate "unsigned" flag, with test

11

Changed 15 months ago by necaris

  • cc rami.chowdhury@… added
  • keywords review added
  • owner jtanis deleted

12

Changed 15 months 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 15 months ago by necaris

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

Changed 15 months ago by necaris

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

13

Changed 15 months ago by necaris

  • owner necaris deleted
  • keywords review added

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.

14

Changed 15 months ago by thijs

  • keywords review removed
  • owner set to necaris
  • description modified (diff)

Thanks, the warning references 12.2 instead of 12.1 though.

Changed 15 months ago by necaris

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

15

Changed 15 months ago by necaris

  • owner necaris deleted
  • keywords review added

Oops, my mistake -- corrected patch.

16

Changed 15 months ago by cyli

  • owner set to necaris
  • keywords review removed

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 15 months ago by necaris

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

17

Changed 15 months ago by necaris

  • owner necaris deleted
  • keywords review added

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).

18

Changed 15 months ago by cyli

  • branch set to branches/deprecate-tap2rpm-4086
  • branch_author set to cyli

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

19

Changed 15 months ago by cyli

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

refs #4086

20

Changed 15 months ago by cyli

  • keywords review removed

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

Going to merge into trunk.

21

Changed 15 months ago by cyli

  • status changed from new to closed
  • resolution set to fixed

(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

22

Changed 15 months 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.