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

Fixed dedent for tab indent. #4870

Merged
merged 1 commit into from
Feb 2, 2017
Merged

Conversation

felixxm
Copy link
Contributor

@felixxm felixxm commented Jan 31, 2017

Refs #4869

@jpadilla jpadilla changed the title Refs #4869 -- Fixed dedent for tab indent. Fixed dedent for tab indent. Jan 31, 2017
@xordoquy
Copy link
Collaborator

Actually, that breaks the backward compatibility with the older dedent.

I don't get why the second line's tabulation should be removed.
Mind elaborating ?

@felixxm
Copy link
Contributor Author

felixxm commented Feb 1, 2017

We use dedent to create view description from docstring (see e.g. test_description.TestViewNamesAndDescriptions.test_view_description_uses_docstring). IMO it should work in exactly the same way for spaces and tabs indentation, currently is not. First line tabulation was removed only because we have content.strip() in the last line. Maybe additional test make that change more clear e.g.:

diff --git a/tests/test_description.py b/tests/test_description.py
index 001a3ea..54a2aab 100644
--- a/tests/test_description.py
+++ b/tests/test_description.py
@@ -82,6 +82,24 @@ class TestViewNamesAndDescriptions(TestCase):
 
         assert MockView().get_view_description() == DESCRIPTION
 
+        class MockViewTabs(APIView):
+            """an example docstring
+\t\t\t====================
+
+\t\t\t* list
+\t\t\t* list
+
+\t\t\tanother header
+\t\t\t--------------
+
+\t\t\t\tcode block
+
+\t\t\tindented
+
+\t\t\t# hash style header #"""
+
+        assert MockViewTabs().get_view_description() == DESCRIPTION.replace('    ', '\t')
+
     def test_view_description_can_be_empty(self):
         """
         Ensure that if a view has no docstring,

@felixxm
Copy link
Contributor Author

felixxm commented Feb 1, 2017

See also previous issue #4345 (with confusing Expected/Actual behavior section shift) and commit bda16a5.

@kevin-brown
Copy link
Member

Depending on whether or not

result = 'first string\n\nsecond string'
assert dedent("    first string\n\n    second string") == result

Passes or fails on the latest master should determine whether or not the equivalent tab case should pass or fail. Ultimately we want to be consistent, but we also don't want to break backwards compatibility when we don't need to.

Unfortunately I didn't get the chance to test this out on the latest master, which is why I'm leaving this as a comment.

@felixxm
Copy link
Contributor Author

felixxm commented Feb 2, 2017

assert dedent("    first string\n\n    second string") == 'first string\n\nsecond string'

passes on the latest master, please take a look also at related test test_description.TestViewNamesAndDescriptions.test_view_description_uses_docstring.

I wouldn't say that PR breaks backward compatibility, because dedent as well as get_view_description is not working currently properly for tab indentation. It was classified as a bug in #4345 and bda16a5 didn't fix it. bda16a5 added dead code as I explained in #4869 (see formatting.py#L48-L50 and coverage report).

@xordoquy
Copy link
Collaborator

xordoquy commented Feb 2, 2017

I was not sure what the expected behavior from dedent was. #4345 clearly states it isn't working as expected for tabs. Will requalify this once I have access to my computer. Thanks for the clarifications.

@xordoquy xordoquy added this to the 3.5.4 Release milestone Feb 2, 2017
@xordoquy xordoquy merged commit f4707ad into encode:master Feb 2, 2017
@xordoquy
Copy link
Collaborator

xordoquy commented Feb 2, 2017

Thanks for taking the time to fix it and supporting it.

@felixxm felixxm deleted the issue-dedent branch February 2, 2017 11:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants