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.

Sign up for GitHub

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

STORM-2339: Python code format cleanup in storm.py #1918

Closed
wants to merge 1 commit into from
Closed

STORM-2339: Python code format cleanup in storm.py #1918

wants to merge 1 commit into from

Conversation

tibkiss
Copy link
Contributor

@tibkiss tibkiss commented Feb 3, 2017

No description provided.

@tibkiss
Copy link
Contributor Author

tibkiss commented Feb 24, 2017

This PR has been pending since a while now.
Could someone please take a look?

Thanks!

@harshach
Copy link
Contributor

+1 on code. @tibkiss please change the title of the JIRA and commit text to something meaningful like "Python code format cleanup in storm.py".

@tibkiss tibkiss changed the title STORM-2339: Beauty contest in storm.py STORM-2339: Python code format cleanup in storm.py Feb 25, 2017
@tibkiss
Copy link
Contributor Author

tibkiss commented Feb 25, 2017

Thanks for the review @harshach , I've changed the title in JIRA, PR & commit message accordingly.

@tibkiss
Copy link
Contributor Author

tibkiss commented Mar 8, 2017

bump

HeartSaVioR
HeartSaVioR reviewed Mar 8, 2017
View reviewed changes
Copy link
Contributor

@HeartSaVioR HeartSaVioR left a comment

Choose a reason for hiding this comment

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

Looks good overall. +1

bin/storm.py
storm_log_dir = confvalue("storm.log.dir",[CLUSTER_CONF_DIR])
if(storm_log_dir == None or storm_log_dir == "nil"):

def exec_storm_class(klass, jvmtype="-server", jvmopts=None, extrajars=None, args=None, fork=False, daemon=True,
Copy link
Contributor

Choose a reason for hiding this comment

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

Just curious: I know it works, but would like to see why you avoid having [] as default value.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Glad you asked. Specifying empty list as default gets initialized once, therefore multiple calls to the function will accumulate the appended values to the list. More lengthy description of this 'feature' could be found here: http://effbot.org/zone/default-values.htm

Copy link
Contributor

Choose a reason for hiding this comment

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

Wow... I didn't know its behavior completely even I worked with Python for 2 years! (4 years ago indeed...) Feels like far from its definition... Thanks for sharing.

@tibkiss
Copy link
Contributor Author

tibkiss commented Apr 29, 2017

"At 2 months, your baby doesn’t yet have the coordination to play with toys. But she may bat at a colorful object hanging in front of her. Your baby may even briefly hold a toy that you place in one of her hands."

Could someone please merge this before it starts to play with toys? Thanks.

@HeartSaVioR
Copy link
Contributor

@tibkiss Sorry forgot this completely. I'll merge this.

@HeartSaVioR
Copy link
Contributor

@tibkiss
But not against 1.0.x branch since I don't mind that version line any more. I'll merge this to master and 1.x branch. Please let me know if you really would like to merge this to 1.0.x branch.

@HeartSaVioR
Copy link
Contributor

@tibkiss
Unfortunately too much changes have been applied to both master and 1.x branch, hence lots of merge conflicts occur.
I'm really sorry but could you work on this for master and 1.x-branch?

And please let me know if you adopt based on some guides like PEP8. We've introduced style guide for Java, and maybe we could introduce one for Python.

@tibkiss
Copy link
Contributor Author

tibkiss commented Jun 23, 2017

This is unfortunate.
Maybe someone else can take this on: My eagerness evaporated somewhere around April.

@HeartSaVioR
Copy link
Contributor

@tibkiss I understand. Let's close this for now. Maybe along with a consistent style guide would be also better for python code, though we only use python for startup script.

d2r pushed a commit to d2r/storm that referenced this pull request Oct 16, 2018
Closes Pull Requests unchanged in 2018
fcda81c 
We are closing stale Pull Requests to make the list more manageable.

Please re-open any Pull Request that has been closed in error.

Closes apache#608
Closes apache#639
Closes apache#640
Closes apache#648
Closes apache#662
Closes apache#668
Closes apache#692
Closes apache#705
Closes apache#724
Closes apache#728
Closes apache#730
Closes apache#753
Closes apache#803
Closes apache#854
Closes apache#922
Closes apache#986
Closes apache#992
Closes apache#1019
Closes apache#1040
Closes apache#1041
Closes apache#1043
Closes apache#1046
Closes apache#1051
Closes apache#1078
Closes apache#1146
Closes apache#1164
Closes apache#1165
Closes apache#1178
Closes apache#1213
Closes apache#1225
Closes apache#1258
Closes apache#1259
Closes apache#1268
Closes apache#1272
Closes apache#1277
Closes apache#1278
Closes apache#1288
Closes apache#1296
Closes apache#1328
Closes apache#1342
Closes apache#1353
Closes apache#1370
Closes apache#1376
Closes apache#1391
Closes apache#1395
Closes apache#1399
Closes apache#1406
Closes apache#1410
Closes apache#1422
Closes apache#1427
Closes apache#1443
Closes apache#1462
Closes apache#1468
Closes apache#1483
Closes apache#1506
Closes apache#1509
Closes apache#1515
Closes apache#1520
Closes apache#1521
Closes apache#1525
Closes apache#1527
Closes apache#1544
Closes apache#1550
Closes apache#1566
Closes apache#1569
Closes apache#1570
Closes apache#1575
Closes apache#1580
Closes apache#1584
Closes apache#1591
Closes apache#1600
Closes apache#1611
Closes apache#1613
Closes apache#1639
Closes apache#1703
Closes apache#1711
Closes apache#1719
Closes apache#1737
Closes apache#1760
Closes apache#1767
Closes apache#1768
Closes apache#1785
Closes apache#1799
Closes apache#1822
Closes apache#1824
Closes apache#1844
Closes apache#1874
Closes apache#1918
Closes apache#1928
Closes apache#1937
Closes apache#1942
Closes apache#1951
Closes apache#1957
Closes apache#1963
Closes apache#1964
Closes apache#1965
Closes apache#1967
Closes apache#1968
Closes apache#1971
Closes apache#1985
Closes apache#1986
Closes apache#1998
Closes apache#2031
Closes apache#2032
Closes apache#2071
Closes apache#2076
Closes apache#2108
Closes apache#2119
Closes apache#2128
Closes apache#2142
Closes apache#2174
Closes apache#2206
Closes apache#2297
Closes apache#2322
Closes apache#2332
Closes apache#2341
Closes apache#2377
Closes apache#2414
Closes apache#2469
d2r pushed a commit to d2r/storm that referenced this pull request Oct 16, 2018
Closes Pull Requests unchanged in 2018
fdb95d0 
We are closing stale Pull Requests to make the list more manageable.

Please re-open any Pull Request that has been closed in error.

Closes apache#608
Closes apache#639
Closes apache#640
Closes apache#648
Closes apache#662
Closes apache#668
Closes apache#692
Closes apache#705
Closes apache#724
Closes apache#728
Closes apache#730
Closes apache#753
Closes apache#803
Closes apache#854
Closes apache#922
Closes apache#986
Closes apache#992
Closes apache#1019
Closes apache#1040
Closes apache#1041
Closes apache#1043
Closes apache#1046
Closes apache#1051
Closes apache#1078
Closes apache#1146
Closes apache#1164
Closes apache#1165
Closes apache#1178
Closes apache#1213
Closes apache#1225
Closes apache#1258
Closes apache#1259
Closes apache#1268
Closes apache#1272
Closes apache#1277
Closes apache#1278
Closes apache#1288
Closes apache#1296
Closes apache#1328
Closes apache#1342
Closes apache#1353
Closes apache#1370
Closes apache#1376
Closes apache#1391
Closes apache#1395
Closes apache#1399
Closes apache#1406
Closes apache#1410
Closes apache#1422
Closes apache#1427
Closes apache#1443
Closes apache#1462
Closes apache#1468
Closes apache#1483
Closes apache#1506
Closes apache#1509
Closes apache#1515
Closes apache#1520
Closes apache#1521
Closes apache#1525
Closes apache#1527
Closes apache#1544
Closes apache#1550
Closes apache#1566
Closes apache#1569
Closes apache#1570
Closes apache#1575
Closes apache#1580
Closes apache#1584
Closes apache#1591
Closes apache#1600
Closes apache#1611
Closes apache#1613
Closes apache#1639
Closes apache#1703
Closes apache#1711
Closes apache#1719
Closes apache#1737
Closes apache#1760
Closes apache#1767
Closes apache#1768
Closes apache#1785
Closes apache#1799
Closes apache#1822
Closes apache#1824
Closes apache#1844
Closes apache#1874
Closes apache#1918
Closes apache#1928
Closes apache#1937
Closes apache#1942
Closes apache#1951
Closes apache#1957
Closes apache#1963
Closes apache#1964
Closes apache#1965
Closes apache#1967
Closes apache#1968
Closes apache#1971
Closes apache#1985
Closes apache#1986
Closes apache#1998
Closes apache#2031
Closes apache#2032
Closes apache#2071
Closes apache#2076
Closes apache#2108
Closes apache#2119
Closes apache#2128
Closes apache#2142
Closes apache#2174
Closes apache#2206
Closes apache#2297
Closes apache#2322
Closes apache#2332
Closes apache#2341
Closes apache#2377
Closes apache#2414
Closes apache#2469
@d2r d2r mentioned this pull request Oct 16, 2018
Merged
@asfgit asfgit closed this in #2880 Oct 22, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@HeartSaVioR HeartSaVioR HeartSaVioR left review comments

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

Successfully merging this pull request may close these issues.
3 participants
@tibkiss @harshach @HeartSaVioR