ppannuto / python-titlecase

Python library to capitalize strings as specified by the New York Times Manual of Style
MIT License
244 stars 36 forks source link

Custom abbreviations not working on Windows with Python 3.5/3.6/3.7 #86

Closed brocksam closed 3 years ago

brocksam commented 3 years ago

As highlighted by @fireundubh in a comment on PR #84, due to a PermissionError. This error is generated when using both Regex and the optional fallback to re from the standard library. This is a Windows-only error and only affects using Titlecase with custom abbreviations.

The PermissionError occurs in the test test_custom_abbreviations. An example output from the GitHub Actions workflow is shown below:

======================================================================
ERROR: titlecase.tests.test_custom_abbreviations
----------------------------------------------------------------------
Traceback (most recent call last):
  File "c:\hostedtoolcache\windows\python\3.5.4\x64\lib\site-packages\nose\case.py", line 198, in runTest
    self.test(*self.arg)
  File "D:\a\python-titlecase\python-titlecase\titlecase\tests.py", line 383, in test_custom_abbreviations
    assert titlecase('sending UDP packets over PPPoE works great', callback=create_wordlist_filter_from_file(f.name)) == 'Sending UDP Packets Over PPPoE Works Great'
  File "D:\a\python-titlecase\python-titlecase\titlecase\__init__.py", line 216, in create_wordlist_filter_from_file
    with open(file_path_str) as f:
PermissionError: [Errno 13] Permission denied: 'C:\\Users\\RUNNER~1\\AppData\\Local\\Temp\\tmp8iadjc2i'
-------------------- >> begin captured logging << --------------------
titlecase: DEBUG: Sending UDP Packets Over PPPoE Works Great
titlecase: DEBUG: Sending Udp Packets Over Pppoe Works Great
--------------------- >> end captured logging << ---------------------
fireundubh commented 3 years ago

Despite my previous PR, I don't think the issue should be fixed in the create_wordlist_filter_from_file function. The test creates a tempfile.NamedTemporaryFile and this is only ever done in the test itself.

It would make more sense to have custom abbreviations test data in a file and pass in the file path instead - perhaps via a mock argparse.Namespace object. This would be more in line with how create_wordlist_filter_from_file is actually used.

Alternatively, the test itself can be rewritten:

def test_custom_abbreviations():
    f = tempfile.NamedTemporaryFile(mode='w', delete=False)  # do not delete on close
    # <snip>
    f.close()  # manually close
    os.unlink(f.name)  # manually delete

The test will then pass on Windows.

brocksam commented 3 years ago

Alternatively, the test itself can be rewritten:

def test_custom_abbreviations():
    f = tempfile.NamedTemporaryFile(mode='w', delete=False)  # do not delete on close
    # <snip>
    f.close()  # manually close
    os.unlink(f.name)  # manually delete

Implemented as part of PR #85. Tests now passing on Windows with all Python versions. Thanks @fireundubh. This issue can be closed upon merging PR #85.

ppannuto commented 3 years ago

Fixed by #85.