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

Fix: if you change the result object, python runner wouldn't return any results #503

Merged
merged 3 commits into from Jul 22, 2015

Conversation

arikfr
Copy link
Member

@arikfr arikfr commented Jul 22, 2015

No description provided.


json_data = json.dumps(self._result)
Copy link
Member Author

Choose a reason for hiding this comment

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

@erans FYI.

The problem was here: if you set a different object to result in the script, then self._result will no longer reference to it.

Copy link
Contributor

Choose a reason for hiding this comment

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

I had to do it due to scoping issue and the support for the print statement

On Wed, Jul 22, 2015, 17:37 Arik Fraimovich notifications@github.com
wrote:

In redash/query_runner/python.py
#503 (comment):

  •        json_data = json.dumps(self._result)
    

@erans https://github.com/erans FYI.

The problem was here: if you set a different object to result in the
script, then self._result will no longer reference to it.


Reply to this email directly or view it on GitHub
https://github.com/EverythingMe/redash/pull/503/files#r35219495.

Copy link
Contributor

Choose a reason for hiding this comment

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

Perhaps I can add a statement to override it.

On Wed, Jul 22, 2015, 17:39 Eran Sandler eran@sandler.co.il wrote:

I had to do it due to scoping issue and the support for the print statement

On Wed, Jul 22, 2015, 17:37 Arik Fraimovich notifications@github.com
wrote:

In redash/query_runner/python.py
#503 (comment):

  •        json_data = json.dumps(self._result)
    

@erans https://github.com/erans FYI.

The problem was here: if you set a different object to result in the
script, then self._result will no longer reference to it.


Reply to this email directly or view it on GitHub
https://github.com/EverythingMe/redash/pull/503/files#r35219495.

Copy link
Member Author

Choose a reason for hiding this comment

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

Actually it's better to accumulate the log statements in another variable, and then put them in result["log"], or they might overriden -- I'll do it in this branch.

Copy link
Contributor

Choose a reason for hiding this comment

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

OK but in that case it may override anything that was set in result["log"]

On Wed, Jul 22, 2015, 17:47 Arik Fraimovich notifications@github.com
wrote:

In redash/query_runner/python.py
#503 (comment):

  •        json_data = json.dumps(self._result)
    

Actually it's better to accumulate the log statements in another variable,
and then put them in result["log"], or they might overriden -- I'll do it
in this branch.


Reply to this email directly or view it on GitHub
https://github.com/EverythingMe/redash/pull/503/files#r35220733.

Copy link
Member Author

Choose a reason for hiding this comment

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

I could merge if it exists already, but doesn't feel like it worth the hassle. People shouldn't abuse this :)

See fix here: 07b88d0
(removed the need for WeakRef too)

Copy link
Contributor

Choose a reason for hiding this comment

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

OK. Looks good.

On Wed, Jul 22, 2015, 17:58 Arik Fraimovich notifications@github.com
wrote:

In redash/query_runner/python.py
#503 (comment):

  •        json_data = json.dumps(self._result)
    

I could merge if it exists already, but doesn't feel like it worth the
hassle. People shouldn't abuse this :)

See fix here: 07b88d0
07b88d0
(removed the need for WeakRef too)


Reply to this email directly or view it on GitHub
https://github.com/EverythingMe/redash/pull/503/files#r35222171.

arikfr added a commit that referenced this pull request Jul 22, 2015
Fix: if you change the result object, python runner wouldn't return any results
@arikfr arikfr merged commit 5feb563 into master Jul 22, 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
Development

Successfully merging this pull request may close these issues.

None yet

2 participants