You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@storm.apache.org by tibkiss <gi...@git.apache.org> on 2017/02/03 11:18:52 UTC
[GitHub] storm pull request #1918: STORM-2339: Beauty contest in storm.py
GitHub user tibkiss opened a pull request:
https://github.com/apache/storm/pull/1918
STORM-2339: Beauty contest in storm.py
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/tibkiss/storm fix/beauty_contest
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/storm/pull/1918.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #1918
----
commit 7d963f31533ec6d5d14b5a64343b80464ac68d2a
Author: Tibor Kiss <ti...@gmail.com>
Date: 2017-02-03T11:16:58Z
STORM-2339: Beauty contest in storm.py
----
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] storm pull request #1918: STORM-2339: Python code format cleanup in storm.py
Posted by HeartSaVioR <gi...@git.apache.org>.
Github user HeartSaVioR commented on a diff in the pull request:
https://github.com/apache/storm/pull/1918#discussion_r105018450
--- Diff: bin/storm.py ---
@@ -193,21 +205,26 @@ def parse_args(string):
args = [re.compile(r"'((?:[^'\\]|\\.)*)'").sub('\\1', x) for x in args]
return [re.compile(r'\\(.)').sub('\\1', x) for x in args]
-def exec_storm_class(klass, jvmtype="-server", jvmopts=[], extrajars=[], args=[], fork=False, daemon=True, daemonName=""):
- global CONFFILE
- 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,
--- End diff --
Just curious: I know it works, but would like to see why you avoid having `[]` as default value.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] storm pull request #1918: STORM-2339: Python code format cleanup in storm.py
Posted by tibkiss <gi...@git.apache.org>.
Github user tibkiss commented on a diff in the pull request:
https://github.com/apache/storm/pull/1918#discussion_r105094892
--- Diff: bin/storm.py ---
@@ -193,21 +205,26 @@ def parse_args(string):
args = [re.compile(r"'((?:[^'\\]|\\.)*)'").sub('\\1', x) for x in args]
return [re.compile(r'\\(.)').sub('\\1', x) for x in args]
-def exec_storm_class(klass, jvmtype="-server", jvmopts=[], extrajars=[], args=[], fork=False, daemon=True, daemonName=""):
- global CONFFILE
- 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,
--- End diff --
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
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] storm issue #1918: STORM-2339: Python code format cleanup in storm.py
Posted by tibkiss <gi...@git.apache.org>.
Github user tibkiss commented on the issue:
https://github.com/apache/storm/pull/1918
"At 2 months, your baby doesn\u2019t 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.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] storm issue #1918: STORM-2339: Python code format cleanup in storm.py
Posted by tibkiss <gi...@git.apache.org>.
Github user tibkiss commented on the issue:
https://github.com/apache/storm/pull/1918
This is unfortunate.
Maybe someone else can take this on: My eagerness evaporated somewhere around April.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] storm issue #1918: STORM-2339: Python code format cleanup in storm.py
Posted by HeartSaVioR <gi...@git.apache.org>.
Github user HeartSaVioR commented on the issue:
https://github.com/apache/storm/pull/1918
@tibkiss Sorry forgot this completely. I'll merge this.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] storm pull request #1918: STORM-2339: Python code format cleanup in storm.py
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/storm/pull/1918
---
[GitHub] storm issue #1918: STORM-2339: Python code format cleanup in storm.py
Posted by HeartSaVioR <gi...@git.apache.org>.
Github user HeartSaVioR commented on the issue:
https://github.com/apache/storm/pull/1918
@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.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] storm issue #1918: STORM-2339: Python code format cleanup in storm.py
Posted by tibkiss <gi...@git.apache.org>.
Github user tibkiss commented on the issue:
https://github.com/apache/storm/pull/1918
bump
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] storm issue #1918: STORM-2339: Beauty contest in storm.py
Posted by tibkiss <gi...@git.apache.org>.
Github user tibkiss commented on the issue:
https://github.com/apache/storm/pull/1918
This PR has been pending since a while now.
Could someone please take a look?
Thanks!
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] storm issue #1918: STORM-2339: Python code format cleanup in storm.py
Posted by HeartSaVioR <gi...@git.apache.org>.
Github user HeartSaVioR commented on the issue:
https://github.com/apache/storm/pull/1918
@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.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] storm pull request #1918: STORM-2339: Python code format cleanup in storm.py
Posted by HeartSaVioR <gi...@git.apache.org>.
Github user HeartSaVioR commented on a diff in the pull request:
https://github.com/apache/storm/pull/1918#discussion_r105096961
--- Diff: bin/storm.py ---
@@ -193,21 +205,26 @@ def parse_args(string):
args = [re.compile(r"'((?:[^'\\]|\\.)*)'").sub('\\1', x) for x in args]
return [re.compile(r'\\(.)').sub('\\1', x) for x in args]
-def exec_storm_class(klass, jvmtype="-server", jvmopts=[], extrajars=[], args=[], fork=False, daemon=True, daemonName=""):
- global CONFFILE
- 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,
--- End diff --
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.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] storm issue #1918: STORM-2339: Python code format cleanup in storm.py
Posted by tibkiss <gi...@git.apache.org>.
Github user tibkiss commented on the issue:
https://github.com/apache/storm/pull/1918
Thanks for the review @harshach , I've changed the title in JIRA, PR & commit message accordingly.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] storm issue #1918: STORM-2339: Beauty contest in storm.py
Posted by harshach <gi...@git.apache.org>.
Github user harshach commented on the issue:
https://github.com/apache/storm/pull/1918
+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".
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] storm issue #1918: STORM-2339: Python code format cleanup in storm.py
Posted by HeartSaVioR <gi...@git.apache.org>.
Github user HeartSaVioR commented on the issue:
https://github.com/apache/storm/pull/1918
@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.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---