Skip to content

Conversation

@silentsokolov
Copy link
Contributor

What changes were proposed in this pull request?

Fix minor typos python example code in streaming programming guide

How was this patch tested?

N/A

@srowen
Copy link
Member

srowen commented Aug 25, 2016

OK, can you perhaps quickly search for other instances of the same in Python code? it's worth a skim if you're up for it.

@srowen
Copy link
Member

srowen commented Aug 25, 2016

Jenkins test this please

@SparkQA
Copy link

SparkQA commented Aug 25, 2016

Test build #64414 has finished for PR 14805 at commit 92310d9.

  • This patch passes all tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

@silentsokolov
Copy link
Contributor Author

I looked through all the python code examples, fixed typos and formatting

Copy link
Member

Choose a reason for hiding this comment

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

Yeah I think it's good to standardize strings on double quotes, unless I'm missing some important difference. There are some other good clear fixes here. The whitespace changes are probably not worth it but I'd leave them in. What about this though, is it really more standard not to precede the continuation with a space?

Copy link
Contributor Author

@silentsokolov silentsokolov Aug 26, 2016

Choose a reason for hiding this comment

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

@srowen, there is no clear definition of it.

https://docs.python.org/2/reference/lexical_analysis.html#explicit-line-joining
https://www.python.org/dev/peps/pep-0008/#maximum-line-length

I use:

# with space
if 1900 < year < 2100 and 1 <= month <= 12:
    return 1

if 1900 < year < 2100 \
    and 1 <= month <= 12:
    return 1

# without space
s = 'Hello world'.lower().split()

s = 'Hello world'.lower()\
    .split()

This makes the code more readable. Or return to the code spaces?

Copy link
Contributor

@gsemet gsemet Aug 26, 2016

Choose a reason for hiding this comment

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

Using simple or double quote have impact on Python, it's up to the developer's will.

Adding a space before the '' makes sense, even though I prefere the following syntax for RDD filters:

socketDF = (spark
            .readStream()
            .format("socket")
            .option("host", "localhost")
            .option("port", 9999)
            .load())

@srowen
Copy link
Member

srowen commented Aug 26, 2016

@stibbons since you're around, do you have any comments on these Python style changes?

@gsemet
Copy link
Contributor

gsemet commented Aug 26, 2016

Look good to me. I think it good to have the space arround kwargs assignation removed to distinguish beween

a = 1

and

afunc(a=1)

which is a good habit to take (and PEP8)

@gsemet
Copy link
Contributor

gsemet commented Aug 26, 2016

For information, autopep8 will do all the space changes automatically on python files, and we can ask sphinx to automatically pep8 (check) autopep8 (fix some pep issues) all python codes

@silentsokolov
Copy link
Contributor Author

silentsokolov commented Aug 26, 2016

I returned the spaces (in all python examples)

@srowen
Copy link
Member

srowen commented Aug 26, 2016

OK, it's looking good to me. I wouldn't mind a set of eyes more familiar with Python to look at it. In particular just want to make sure we're headed towards more standardization in all cases.

@srowen
Copy link
Member

srowen commented Aug 29, 2016

Jenkins retest this please

@SparkQA
Copy link

SparkQA commented Aug 29, 2016

Test build #64564 has finished for PR 14805 at commit 36c99f8.

  • This patch passes all tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

@srowen
Copy link
Member

srowen commented Aug 30, 2016

Merged to master

@asfgit asfgit closed this in d4eee99 Aug 30, 2016
@silentsokolov silentsokolov deleted the fix-typos branch August 30, 2016 10:49
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