Skip to content

Use Jinja2 tojson filter #1101

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

Merged
merged 3 commits into from
Mar 18, 2019
Merged

Conversation

Conengmo
Copy link
Member

Don't json.dumps options and data, but use Jinja2's tojson filter to convert Python variables to JS compatible formats. This works for strings, numbers, bools, lists and dictionaries. I added a test file to verify this behavior.

Normalize the behavior of options. In __init__, put options in a self.options dictionary using a new parse_options() function. Also allow **kwargs whenever it makes sense.

This combination: consistent option handling and using tojson makes our code and templates more clean and consistent. Allowing **kwargs gives users more flexibility.

I also did refactoring of templates whenever I touched them. Have consistent indentation and style.

Fixed the tests for the changes. Consistently use the normalize function to normalize rendered output.

""")
{% macro script(this, kwargs) %}
var {{ this.get_name() }} = new L.RegularPolygonMarker(
{{ this.location|tojson }},
Copy link
Member

Choose a reason for hiding this comment

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

Way cleaner! I like this!!

@ocefpaf
Copy link
Member

ocefpaf commented Mar 17, 2019

@Conengmo this is failing legacy Python and the notebook tests. The former we no longer care, #1100 was already merged BTW, but the latter seems to be related to the changes here, right?

@Conengmo
Copy link
Member Author

I’ll have a look and fix the notebooks. After rebasing Python 2 should no longer be a problem :)

This PR touches quite a lot of code. Do you see any problems with this?

@ocefpaf
Copy link
Member

ocefpaf commented Mar 17, 2019

I’ll have a look and fix the notebooks. After rebasing Python 2 should no longer be a problem :)

This PR touches quite a lot of code. Do you see any problems with this?

At a first glance, no. Even though it does touch a lot of the code base it is virtually the same changes everywhere, so let's trust our tests and merge this as soon as you rebase it and the tests are passing.

@Conengmo
Copy link
Member Author

let's trust our tests

Good point! During the work for this PR I looked at the tests more, and so I got thinking about want to look into having better functional tests.

@Conengmo
Copy link
Member Author

Conengmo commented Mar 17, 2019

@ocefpaf seems the Travis build is failing on the installation step. It started from the last merged PR. It's hard to debug because it sees the whole block as one command:

The command "wget http://bit.ly/miniconda -O miniconda.sh
  bash miniconda.sh -b -p $HOME/miniconda
  export PATH="$HOME/miniconda/bin:$PATH"
  conda config --set always_yes yes --set changeps1 no --set show_channel_urls true
  conda update conda --quiet
  conda config --add channels conda-forge --force
  conda install pycryptosat
  conda config --set channel_priority strict
  conda config --set safety_checks disabled
  conda create --name TEST python=$PY --file requirements.txt --file requirements-dev.txt
  source activate TEST
  " failed and exited with 1 during .

Don't `json.dumps` options and data, but use Jinja2's `tojson` filter to convert Python variables to JS compatible formats. This works for strings, numbers, bools, lists and dictionaries. I added a test file to verify this behavior.

Normalize the behavior of options. In `__init__`, put options in a `self.options` dictionary using a new `parse_options()` function. Also allow `**kwargs` whenever it makes sense.

I also did refactoring of templates whenever I touched them. Have consistent indentation and style.

Fixed the tests for the changes. Consistently use the `normalize` function to normalize rendered output.
If Numpy int32's are returned the `tojson` filter doesn't work.
@ocefpaf
Copy link
Member

ocefpaf commented Mar 17, 2019

@ocefpaf seems the Travis build is failing on the installation step. It started from the last merged PR. It's hard to debug because it sees the whole block as one command:

investigating it. Hold on...

@ocefpaf
Copy link
Member

ocefpaf commented Mar 18, 2019

@Conengmo the CIs are back online and the failure there is real ;-p

@Conengmo
Copy link
Member Author

Thanks for bringing the CI back! I see the test is indeed failing. It's due to the Webmapservice that's used in the WMS example notebook. This has happened before, now it seems to give a HTTP 503 instead of a timeout. I'll see if we can use another URL...

@ocefpaf
Copy link
Member

ocefpaf commented Mar 18, 2019

Thanks for bringing the CI back! I see the test is indeed failing. It's due to the Webmapservice that's used in the WMS example notebook. This has happened before, now it seems to give a HTTP 503 instead of a timeout. I'll see if we can use another URL...

Or we can just add an xfail (expected failure) there. I'll see if I can do that later today. It is unrelated to this PRs anyway so let's get this merged!

@ocefpaf ocefpaf merged commit 36e4118 into python-visualization:master Mar 18, 2019
@Conengmo Conengmo deleted the jinja2-tojson branch March 18, 2019 19:49
@Conengmo Conengmo mentioned this pull request Jan 4, 2023
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.

3 participants