Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Don’t use doctests anymore #2703

Merged
merged 3 commits into from Mar 31, 2017
Merged

Don’t use doctests anymore #2703

merged 3 commits into from Mar 31, 2017

Conversation

Kwpolska
Copy link
Member

Doctests are pretty fragile, require string trickery, and are frowned upon by many other people. Let’s drop them. This commit disables doctests and re-adds all the useful ones as regular unit tests.

@Kwpolska
Don’t use doctests anymore
fb7a267 
Signed-off-by: Chris Warrick <kwpolska@gmail.com>
@Kwpolska
Replace backslash on Windows in path tests
1ff860a 
Signed-off-by: Chris Warrick <kwpolska@gmail.com>
@Kwpolska Kwpolska requested a review from ralsina March 29, 2017 19:15
felixfontein
felixfontein reviewed Mar 31, 2017
View reviewed changes
nikola/utils.py
@@ -469,9 +469,8 @@ class TemplateHookRegistry(object):
>>> r = TemplateHookRegistry('foo', None)
Copy link
Contributor

Choose a reason for hiding this comment

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

Isn't this still a doctest? Or do you want to keep it as documentation?

Copy link
Member Author

Choose a reason for hiding this comment

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

I didn’t remove any doctests, because they’re still pretty good documentation.

felixfontein
felixfontein approved these changes Mar 31, 2017
View reviewed changes
Copy link
Contributor

@felixfontein felixfontein left a comment

Choose a reason for hiding this comment

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

LGTM.

Out of curiosity: what exactly are the problems with doctests? I searched a bit on the net, but mostly found discussions from 2008 or so, so I was wondering whether there are new reasons.

@Kwpolska
Clean up some doctest hacks
0917282 
Signed-off-by: Chris Warrick <kwpolska@gmail.com>
@Kwpolska
Copy link
Member Author

Doctest suck because they’re just stringifying the return value and comparing it using string equality. Therefore:

  1. If the code is compatible with Python 2 and 3, b'' or u'' might appear in repr() of lists (cf. str.join() hacks in get_crumbs I just removed) — that will not pass ==.
  2. You can’t reasonably do environment-dependent things (cf. paths in get_asset_path which had to use a special ELLIPSIS flag and /.../ — the leading slash was needed because ... means code continuation line.
  3. And because dicts/sets have no order, printing either can mean random doctest failures. You need to compare with foo() == {'some': 'dict'} or use sorted in the doctest. Which beats the point of doctests.

Merging. (Ignore the test failures — that’s invariance, caused by this branch being created pre-v7.8.4)

@Kwpolska Kwpolska merged commit c8be300 into master Mar 31, 2017
@Kwpolska Kwpolska deleted the drop-doctests branch March 31, 2017 17:53
@felixfontein
Copy link
Contributor

Ah, so nothing really new.

@Kwpolska Kwpolska mentioned this pull request May 7, 2017
Merged
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@felixfontein felixfontein felixfontein approved these changes

@ralsina ralsina Awaiting requested review from ralsina

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@Kwpolska @felixfontein