Opened 5 years ago

Closed 4 years ago

#5925 task closed fixed (fixed)

Deprecate twisted.python.runtime.Platform.isWinNT

Reported by: Jean-Paul Calderone Owned by: Thijs Triemstra
Priority: lowest Milestone:
Component: core Keywords: win32
Cc: Thijs Triemstra Branch: branches/deprecate-iswinnt-5925
branch-diff, diff-cov, branch-cov, buildbot
Author: thijs

Description

Twisted only supports Windows NT-derived platforms at this point (Windows XP, Windows 7). See <http://en.wikipedia.org/wiki/Windows_NT>.

This function can only ever return true in a supported environment.

Change History (14)

comment:1 Changed 5 years ago by Thijs Triemstra

Owner: set to Thijs Triemstra
Status: newassigned

It was only used once, in python.win32.getProgramsMenuPath which should be replaced once isWinNT is removed.

comment:2 Changed 5 years ago by Thijs Triemstra

(In [36891]) deprecate isWinNT, refs #5925

comment:3 Changed 5 years ago by Thijs Triemstra

Author: thijs
Branch: branches/deprecate-iswinnt-5925
Cc: Thijs Triemstra added
Keywords: review added
Owner: Thijs Triemstra deleted
Status: assignednew

comment:4 Changed 4 years ago by Tom Prince

Keywords: review removed
Owner: set to Thijs Triemstra
  1. If this only returns True on win32, is there any reason to do anything more complicated than return True there? (In which case we can test that fact)
  2. test_isWinNT shouldn't emit a deprecation warning.
  3. python.win32.getProgramsMenuPath should be fixed to not use isWinNT *before* before the deprecation of isWinNT (Since it currently lacks tests, this should perhaps be a separate ticket).
  4. Consider using SynchronousTestCase.callDeprecated for testing the deprecation.
  5. Consider leaving a comment that t.p.deprecate isn't used, to avoid a dependency on that module.

Please resubmit for review after addressing the above issues.

comment:5 Changed 4 years ago by Tom Prince

Also, assertIn is preferable to assertTrue(... in ...).

comment:6 Changed 4 years ago by Jean-Paul Calderone

Consider using SynchronousTestCase.callDeprecated for testing the deprecation

Development documentation currently recommends flushWarnings, not callDeprecated.

comment:7 Changed 4 years ago by Thijs Triemstra

Status: newassigned

There are also some "nt" references in twisted/python/dist.py but that should probably be fixed in separate ticket.

comment:8 Changed 4 years ago by Thijs Triemstra

(In [37117]) address review comments. refs #5925

comment:9 in reply to:  4 Changed 4 years ago by Thijs Triemstra

Keywords: review added
Owner: Thijs Triemstra deleted
Status: assignednew

Replying to tom.prince:

  1. If this only returns True on win32, is there any reason to do anything more complicated than return True there? (In which case we can test that fact)

Replaced the logic with a simple isWindows() check.

  1. test_isWinNT shouldn't emit a deprecation warning.

Fixed.

  1. python.win32.getProgramsMenuPath should be fixed to not use isWinNT *before* before the deprecation of isWinNT (Since it currently lacks tests, this should perhaps be a separate ticket).

Fixed. Also added tests.

Additionally added a test for Platform.isKnown and added some missing docstrings.

Forced a build.

comment:10 Changed 4 years ago by Tom Prince

Keywords: review removed
Owner: set to Thijs Triemstra
  1. It looks like there is one relevant twistedchecker error
    ************* Module twisted.python.test.test_win32
    W9015: 68,0: Too many blank lines, expected 2
    
  2. There are a bunch of \rs in that file.
  3. (optional) I think twisted.python.win32.getProgramsMenuPath can probably be allowed to fail on non-windows. i.e. just remove the conditional there. I guess that is an incompatible change though.
  4. assertNotNull doesn't exists. The tests probably want to at least assert that the return value is a C{str}, since that is what we document.

Please resubmit after addressing the above issues.

comment:11 Changed 4 years ago by Thijs Triemstra

(In [37120]) address review comments. refs #5925

comment:12 in reply to:  10 Changed 4 years ago by Thijs Triemstra

Keywords: review added
Owner: Thijs Triemstra deleted

Replying to tom.prince:

  1. It looks like there is one relevant twistedchecker error
    ************* Module twisted.python.test.test_win32
    W9015: 68,0: Too many blank lines, expected 2
    

Removed. Tried forcing a new build but the process just hung.. Need to get twistedchecker installed locally.

  1. There are a bunch of \rs in that file.

Removed those. Seems these type of changes don't show up in the trac browser.

  1. (optional) I think twisted.python.win32.getProgramsMenuPath can probably be allowed to fail on non-windows. i.e. just remove the conditional there. I guess that is an incompatible change though.

I'd rather handle that in a different ticket (or deprecate these altogether).

  1. assertNotNull doesn't exists. The tests probably want to at least assert that the return value is a C{str}, since that is what we document.

Used assertIsInstance instead.

comment:13 Changed 4 years ago by Tom Prince

Keywords: review removed
Owner: set to Thijs Triemstra

Looks good. Please merge after the windows builders complete. (There are some unrelated failure on those builders, so check that the tests of the code modified here pass).

comment:14 Changed 4 years ago by Thijs Triemstra

Resolution: fixed
Status: newclosed

(In [37185]) Merge deprecate-iswinnt-5925: Platform.isWinNT is deprecated in favor of isWindows.

Author: thijs Reviewer: tom.prince Fixes: #5925

Twisted only supports Windows NT-derived platforms at this point (Windows XP, Windows 7).

Note: See TracTickets for help on using tickets.