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

Added D403: First word of first line should be properly capitalized #165

Merged
merged 6 commits into from Dec 13, 2015

Conversation

@Nurdok
Copy link
Member

@Nurdok Nurdok commented Dec 11, 2015

The new error is turned on by default.

"""Return the source code for the definition."""
full_src = self._source[self._slice]

def predicate(line):

This comment has been minimized.

@sigmavirus24

sigmavirus24 Dec 11, 2015
Member

A better name might be nice, e.g., is_comment_or_empty. predicate is a bit too generic.

This comment has been minimized.

@Nurdok

Nurdok Dec 11, 2015
Author Member

Done.

@@ -678,6 +689,10 @@ def to_rst(cls):
'%r, not %r')
D402 = D4xx.create_error('D402', 'First line should not be the function\'s '
'"signature"')
D402 = D4xx.create_error('D402', 'First line should not be the function\'s '

This comment has been minimized.

@sigmavirus24

sigmavirus24 Dec 11, 2015
Member

Why did you duplicate 402 here?

This comment has been minimized.

@Nurdok

Nurdok Dec 11, 2015
Author Member

By mistake, done.

"""
if docstring:
first_word = eval(docstring).split()[0]

This comment has been minimized.

@sigmavirus24

sigmavirus24 Dec 11, 2015
Member

We should use ast.literal_eval instead. I don't think we want people to have docstrings with arbitrary python code to execute in it.

Edit Unless eval is a function defined by pep257 that is safe (of course).

This comment has been minimized.

@Nurdok

Nurdok Dec 11, 2015
Author Member

In this case, we use eval on a tk.STRING token, so it seems like it can't possibly run arbitrary Python code, can it?

This comment has been minimized.

@sigmavirus24

sigmavirus24 Dec 11, 2015
Member

That's a good question. I'm not 100% certain. My familiarity with the tokenizer library isn't so great.

This comment has been minimized.

@myint

myint Dec 12, 2015
Member

@Nurdok, why risk it? Is there some benefit to using eval() over the safer ast.literal_eval() in this case?

This comment has been minimized.

@sigmavirus24

sigmavirus24 Dec 12, 2015
Member

So @Nurdok is correct, what you get back looks like

(3, '"""Biz baz."""', (3, 3), (3, 17), '   """Biz baz."""\n')

So you're doing

eval('"""Biz baz."""\n')  # => 'Biz baz.'

But it's functionally equivalent to ast.literal_eval in this case. I would still prefer ast.literal_eval to be used here, but I'm not as stringently against using eval as I was earlier.

We should probably add a comment regardless to reassure future reviewers as well.

This comment has been minimized.

@Nurdok

Nurdok Dec 12, 2015
Author Member

I changed it to ast.literal_eval, thanks.

@Nurdok
Copy link
Member Author

@Nurdok Nurdok commented Dec 12, 2015

@sigmavirus24 @myint Any more comments before I merge this?

@myint
Copy link
Member

@myint myint commented Dec 12, 2015

Looks good to me!

Thanks

Nurdok added a commit that referenced this pull request Dec 13, 2015
Added D403: First word of first line should be properly capitalized
@Nurdok Nurdok merged commit f7c12a8 into PyCQA:master Dec 13, 2015
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@Nurdok Nurdok deleted the Nurdok:D403 branch Dec 13, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants