Skip to content

Fixed #24399 -- Made filesystem loaders use more specific exceptions.#4229

Closed
prestontimmons wants to merge 1 commit intodjango:masterfrom
prestontimmons:ticket-24399
Closed

Fixed #24399 -- Made filesystem loaders use more specific exceptions.#4229
prestontimmons wants to merge 1 commit intodjango:masterfrom
prestontimmons:ticket-24399

Conversation

@prestontimmons
Copy link
Copy Markdown
Contributor

No description provided.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

os.chmod should be before the try. It doesn't make a big difference but that's the common pattern in general.

@prestontimmons
Copy link
Copy Markdown
Contributor Author

Thanks. I fixed the docs and updated to a tempfile. That's a much better idea.

@aaugustin
Copy link
Copy Markdown
Member

Rebased and merged in 70123cf. Thanks!

@aaugustin aaugustin closed this Mar 3, 2015
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This test is problematic on Windows:

======================================================================
FAIL: test_notafile_error (template_tests.test_loaders.FileSystemLoaderTests)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "c:\Users\Tim\code\django\tests\template_tests\test_loaders.py", line 266
, in test_notafile_error
    self.engine.get_template('first')
AssertionError: "Is\ a\ directory" does not match "[Errno 13] Permission denied:
 u'c:\\Users\\Tim\\code\\django\\tests\\template_tests\\templates\\first'"

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay. I'll take a look into it.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From the testing I did it seems "Permission denied" is the standard message raised on Windows if a directory is passed to open. Since the message can differ per OS, I think the fix is to change assertRaisesMessage(IOError, 'Is a directory') to assertRaises(IOError). I can add a PR if you agree.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does the raised exception have the same errno on both platforms?

If that's the case you could do:

with self.assertRaises(IOError) as ctx:
    # ....
self.assertEqual(ctx.exception.errno, errno.WHATEVER)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's different. Linux raises errno.EISDIR (21). Windows raises errno.EACCES (13).

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems okay to me. I guess the alternative would to vary the message based on OS. Not sure that complexity is required though.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants