Local plot schema by theengineear · Pull Request #648 · plotly/plotly.py

theengineear · 2017-01-05T04:35:15Z

This PR stops auto-loading the plot-schema on import. We decided in #642 that this shouldn't need to force a major version bump, so I'll add a minor version bump here.

See #614 for the original PR. I noticed that we actually had a weird PY3 conflict due to a change in how importing works in PY3 (See commit notes).

Conflicts: plotly/api/v2/plot_schema.py plotly/graph_reference.py plotly/tests/test_core/test_api/test_v2/test_plot_schema.py plotly/tests/test_core/test_graph_reference/test_graph_reference.py

theengineear · 2017-01-05T05:20:39Z

@Kully can you give this a review? We discussed the no-download-on-import portion of this in #642, but I've also done the following (which we haven't discussed wrt Semantic Versioning):

.js and .json files --> plotly/package_data. I can't imagine any users are using the non-python files directly in scripts.
auto-object generation. This isn't backwards-incompatible. In fact, it's exactly what we had before, except now it's not generated on import in memory.

@chriddyp, 👍 on the meta programming introduced in 51ef008?

Kully · 2017-01-05T16:50:18Z

                     for item in items]
-    lines = textwrap.wrap(repr(items_classes), width=LINE_SIZE-TAB_SIZE)
+    items_classes.sort()
+    lines = textwrap.wrap(repr(items_classes), width=LINE_SIZE-TAB_SIZE - 1)


Nit Picky, but would lines = textwrap.wrap(repr(items_classes), width=LINE_SIZE - TAB_SIZE - 1)
or lines = textwrap.wrap(repr(items_classes), width=LINE_SIZE-TAB_SIZE-1) be better for consistency?

Kully · 2017-01-05T16:52:02Z

                    val = val[0]

-                lines = textwrap.wrap(val, width=LINE_SIZE)
+                lines = textwrap.wrap(val, width=LINE_SIZE - 1)


Kully · 2017-01-05T16:52:47Z

+    attributes = list(
+        graph_reference.get_valid_attributes(object_name, parent_object_names))
+    attributes.sort()
+    lines = textwrap.wrap(repr(list(attributes)), width=LINE_SIZE-TAB_SIZE - 1)


See previous comment

Kully · 2017-01-05T17:01:42Z

+
+    :raises: (ValueError) If the FLAG isn't found.
+    :return: (list) The lines we're copying.
+


I think the doc strings should not have the extra space between last line and the triple quotes """

Hehe, I disagree on this one. But I'm OK with changing it. You like to do the blank line after the """ then?

Kully · 2017-01-05T17:23:50Z

+    if not flag_found:
+        raise ValueError(
+            'Failed to find flag:\n''{}\n''in graph_objs_tools.py.'
+            .format(FLAG)


More a suggestion, but for legibility, what do you think about putting the format(FLAG) on the line above? I think it's 81 chars on that line, but would be worth IMO

Your call ;)

Huh... that string looks wrong to me, the extra ' characters? I'll fix this up.

Kully · 2017-01-05T17:26:08Z

+
+
+def print_figure_patch(f):
+    """Print a patch to our Figure object into the given open file."""


For one-liner doc strings, which is more common to your knowledge:

'''one-line-here'''

''' one-line-here '''

I usually do the latter. Let me know if you think this is worth worrying/thinking about

One-liners can/should be one-liners:

https://www.python.org/dev/peps/pep-0257/#one-line-docstrings

Previously, we generated these *on import*, which is slow and it’s a little sneaky. Users end up using classes that are not actually written *anywhere* in our package. Now that we only update the plot schema in version releases (no longer on import), we can also just programmatically add in all the graph obj classes. Instead of adding them to memory, we literally just print them to the file. This is some serious meta-programming… but, the resulting file will have to be syntactically valid and pass our test suite, so I’m not *too* worried about it.

In Python 2, you *must* have an `__init__.py` file to have a python package. However, in Python 3, you don’t need that! We have `/graph_reference` and `graph_reference.py` defined, which seems to *work*, but is poorly organized and should be fixed. For instance, it causes `unresolved import errors` in IDEs that have `goto-source`-type functionality. Conflicts: makefile

This has been a source of confusion every time we end up needing a new resource (like a js or json file). If you forget to add the new pattern in `setup.py`, the installed site-package just won’t include the file. Now all our non `.py` files should live in `package_data/`.

I think this got introduced during some rebases ;__;.

theengineear · 2017-01-05T19:15:32Z

@Kully alright with this?

theengineear · 2017-01-05T19:18:23Z

@chriddyp , it'd be great to get a high-level 👍 from you on 51ef008 still.

You don't need to dig into the code very much. In general I'm just wondering if you think the approach of moving the meta programming we were already doing into auto-generated lines inside the graph_objs.py files seems OK.

Specifically, all the work is done in update_graph_objs.py, where I literally print new lines into graph_objs.py programmatically.

Edit, after rebase --> c26e318

theengineear · 2017-01-05T20:13:56Z

Since I've got a lot of moving parts happening in this repo, I'm going to merge after chatting with @Kully in slack and with @chriddyp's 👍 for hard-coding our graph_objs.

Don’t auto-download graph reference on import.

ef58782

Conflicts: plotly/api/v2/plot_schema.py plotly/graph_reference.py plotly/tests/test_core/test_api/test_v2/test_plot_schema.py plotly/tests/test_core/test_graph_reference/test_graph_reference.py

Kully reviewed Jan 5, 2017

View reviewed changes

theengineear added 5 commits January 5, 2017 09:38

Version bump (no-auto-load-schema) —> 1.13.0

e4e452c

Fix makefile syntax errors.

33754a7

I think this got introduced during some rebases ;__;.

theengineear force-pushed the local-plot-schema branch from 377949e to 33754a7 Compare January 5, 2017 17:44

theengineear merged commit 4191458 into master Jan 5, 2017

theengineear deleted the local-plot-schema branch January 5, 2017 20:14

theengineear mentioned this pull request Jan 5, 2017

No auto load graph reference #643

Closed


		:raises: (ValueError) If the FLAG isn't found.
		:return: (list) The lines we're copying.



		def print_figure_patch(f):
		"""Print a patch to our Figure object into the given open file."""

Uh oh!

Uh oh!

Conversation

theengineear commented Jan 5, 2017

Uh oh!

theengineear commented Jan 5, 2017

Uh oh!

Kully Jan 5, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Kully Jan 5, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Kully Jan 5, 2017

Choose a reason for hiding this comment

Uh oh!

Kully Jan 5, 2017

Choose a reason for hiding this comment

Uh oh!

theengineear Jan 5, 2017

Choose a reason for hiding this comment

Uh oh!

Kully Jan 5, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

theengineear Jan 5, 2017

Choose a reason for hiding this comment

Uh oh!

Kully Jan 5, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

theengineear Jan 5, 2017

Choose a reason for hiding this comment

Uh oh!

theengineear commented Jan 5, 2017

Uh oh!

theengineear commented Jan 5, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

theengineear commented Jan 5, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Kully Jan 5, 2017 •

edited

Loading

Kully Jan 5, 2017 •

edited

Loading

Kully Jan 5, 2017 •

edited

Loading

Kully Jan 5, 2017 •

edited

Loading

theengineear commented Jan 5, 2017 •

edited

Loading