From 8cc1d2ea0ba87f958a02994ef02f5266a5fe9030 Mon Sep 17 00:00:00 2001 From: alexcjohnson Date: Wed, 29 May 2019 11:38:59 -0400 Subject: [PATCH 1/8] remove MutableMapping inheritance from base Component --- dash/development/base_component.py | 4 +- .../dash/development/test_base_component.py | 80 ++++++++----------- 2 files changed, 37 insertions(+), 47 deletions(-) diff --git a/dash/development/base_component.py b/dash/development/base_component.py index 01f8189d3e..6cb9297cd2 100644 --- a/dash/development/base_component.py +++ b/dash/development/base_component.py @@ -59,7 +59,7 @@ def _check_if_has_indexable_children(item): @six.add_metaclass(ComponentMeta) -class Component(patch_collections_abc('MutableMapping')): +class Component(object): class _UNDEFINED(object): def __repr__(self): return 'undefined' @@ -184,7 +184,7 @@ def _get_set_or_delete(self, id, operation, new_item=None): # If we were in a list, then this exception will get caught raise KeyError(id) - # Supply ABC methods for a MutableMapping: + # Magic methods for a mapping interface: # - __getitem__ # - __setitem__ # - __delitem__ diff --git a/tests/unit/dash/development/test_base_component.py b/tests/unit/dash/development/test_base_component.py index d901189ec4..ab1a3ce2f3 100644 --- a/tests/unit/dash/development/test_base_component.py +++ b/tests/unit/dash/development/test_base_component.py @@ -1,5 +1,4 @@ from collections import OrderedDict -import collections import inspect import json import os @@ -8,8 +7,14 @@ import plotly from dash.development.base_component import Component -from dash.development._py_components_generation import generate_class_string, generate_class_file, generate_class, \ - create_docstring, prohibit_events, js_to_py_type +from dash.development._py_components_generation import ( + generate_class_string, + generate_class_file, + generate_class, + create_docstring, + prohibit_events, + js_to_py_type +) Component._prop_names = ('id', 'a', 'children', 'style', ) Component._type = 'TestComponent' @@ -85,8 +90,9 @@ def test_get_item_with_nested_children_two_branches(self): def test_get_item_with_nested_children_with_mixed_strings_and_without_lists(self): # noqa: E501 c, c1, c2, c3, c4, c5 = nested_tree() + keys = [k for k in c] self.assertEqual( - list(c.keys()), + keys, [ '0.0', '0.1', @@ -135,7 +141,8 @@ def test_set_item_with_nested_children_with_mixed_strings_and_without_lists(self def test_del_item_with_nested_children_with_mixed_strings_and_without_lists(self): # noqa: E501 c = nested_tree()[0] - for key in reversed(list(c.keys())): + keys = reversed([k for k in c]) + for key in keys: c[key] del c[key] with self.assertRaises(KeyError): @@ -159,13 +166,6 @@ def test_traverse_with_tuples(self): # noqa: E501 list(c.children) + [c3] + [c2] + list(c2.children) ) - def test_iter_with_nested_children_with_mixed_strings_and_without_lists(self): # noqa: E501 - c = nested_tree()[0] - keys = list(c.keys()) - # get a list of ids that __iter__ provides - iter_keys = [i for i in c] - self.assertEqual(keys, iter_keys) - def test_to_plotly_json_with_nested_children_with_mixed_strings_and_without_lists(self): # noqa: E501 c = nested_tree()[0] Component._namespace @@ -244,17 +244,6 @@ def test_get_item_raises_key_if_id_doesnt_exist(self): with self.assertRaises(KeyError): c3['0'] - def test_equality(self): - # TODO - Why is this the case? How is == being performed? - # __eq__ only needs __getitem__, __iter__, and __len__ - self.assertTrue(Component() == Component()) - self.assertTrue(Component() is not Component()) - - c1 = Component(id='1') - c2 = Component(id='2', children=[Component()]) - self.assertTrue(c1 == c2) - self.assertTrue(c1 is not c2) - def test_set_item(self): c1a = Component(id='1', children='Hello world') c2 = Component(id='2', children=c1a) @@ -450,6 +439,9 @@ def test_len(self): ])), 3) def test_iter(self): + # The mixin methods from MutableMapping were cute but probably never + # used - at least not by us. Test that they're gone + # keys, __contains__, items, values, and more are all mixin methods # that we get for free by inheriting from the MutableMapping # and behave as according to our implementation of __iter__ @@ -469,30 +461,28 @@ def test_iter(self): Component(children=[Component(id='8')]), ] ) - # test keys() - keys = [k for k in list(c.keys())] - self.assertEqual(keys, ['2', '3', '4', '5', '6', '7', '8']) - self.assertEqual([i for i in c], keys) - # test values() - components = [i for i in list(c.values())] - self.assertEqual(components, [c[k] for k in keys]) + mixins = ['clear', 'get', 'items', 'keys', 'pop', 'popitem', + 'setdefault', 'update', 'values'] + + for m in mixins: + self.assertFalse(hasattr(c, m), m) + + keys = ['2', '3', '4', '5', '6', '7', '8'] - # test __iter__() for k in keys: # test __contains__() self.assertTrue(k in c) + # test __getitem__() + self.assertEqual(c[k].id, k) - # test __items__ - items = [i for i in list(c.items())] - self.assertEqual(list(zip(keys, components)), items) + # test __iter__() + keys2 = [] + for k in c: + keys2.append(k) + self.assertIn(k, keys) - def test_pop(self): - c2 = Component(id='2') - c = Component(id='1', children=c2) - c2_popped = c.pop('2') - self.assertTrue('2' not in c) - self.assertTrue(c2_popped is c2) + self.assertEqual(len(keys), len(keys2)) class TestGenerateClassFile(unittest.TestCase): @@ -502,7 +492,7 @@ def setUp(self): with open(json_path) as data_file: json_string = data_file.read() data = json\ - .JSONDecoder(object_pairs_hook=collections.OrderedDict)\ + .JSONDecoder(object_pairs_hook=OrderedDict)\ .decode(json_string) self.data = data @@ -572,7 +562,7 @@ def setUp(self): with open(path) as data_file: json_string = data_file.read() data = json\ - .JSONDecoder(object_pairs_hook=collections.OrderedDict)\ + .JSONDecoder(object_pairs_hook=OrderedDict)\ .decode(json_string) self.data = data @@ -590,7 +580,7 @@ def setUp(self): with open(path) as data_file: json_string = data_file.read() required_data = json\ - .JSONDecoder(object_pairs_hook=collections.OrderedDict)\ + .JSONDecoder(object_pairs_hook=OrderedDict)\ .decode(json_string) self.required_data = required_data @@ -761,7 +751,7 @@ def setUp(self): with open(path) as data_file: json_string = data_file.read() data = json\ - .JSONDecoder(object_pairs_hook=collections.OrderedDict)\ + .JSONDecoder(object_pairs_hook=OrderedDict)\ .decode(json_string) self.data = data @@ -945,7 +935,7 @@ def setUp(self): with open(path) as data_file: json_string = data_file.read() data = json\ - .JSONDecoder(object_pairs_hook=collections.OrderedDict)\ + .JSONDecoder(object_pairs_hook=OrderedDict)\ .decode(json_string) self.data = data From bf26213797c7fc944101fae098c1b0565a57acf7 Mon Sep 17 00:00:00 2001 From: alexcjohnson Date: Wed, 29 May 2019 11:52:16 -0400 Subject: [PATCH 2/8] underscore traverse(_with_paths) methods of base Component --- dash/dash.py | 4 ++-- dash/development/base_component.py | 12 ++++++------ tests/unit/dash/development/test_base_component.py | 4 ++-- 3 files changed, 10 insertions(+), 10 deletions(-) diff --git a/dash/dash.py b/dash/dash.py index 1e31a74ffd..9f5feb0551 100644 --- a/dash/dash.py +++ b/dash/dash.py @@ -914,7 +914,7 @@ def _value_is_valid(val): def _validate_value(val, index=None): # val is a Component if isinstance(val, Component): - for p, j in val.traverse_with_paths(): + for p, j in val._traverse_with_paths(): # check each component value in the tree if not _value_is_valid(j): _raise_invalid( @@ -1194,7 +1194,7 @@ def _validate_layout(self): layout_id = getattr(self.layout, 'id', None) component_ids = {layout_id} if layout_id else set() - for component in to_validate.traverse(): + for component in to_validate._traverse(): component_id = getattr(component, 'id', None) if component_id and component_id in component_ids: raise exceptions.DuplicateIdError( diff --git a/dash/development/base_component.py b/dash/development/base_component.py index 6cb9297cd2..cfd829f61b 100644 --- a/dash/development/base_component.py +++ b/dash/development/base_component.py @@ -208,12 +208,12 @@ def __delitem__(self, id): # pylint: disable=redefined-builtin """Delete items by ID in the tree of children.""" return self._get_set_or_delete(id, 'delete') - def traverse(self): + def _traverse(self): """Yield each item in the tree.""" - for t in self.traverse_with_paths(): + for t in self._traverse_with_paths(): yield t[1] - def traverse_with_paths(self): + def _traverse_with_paths(self): """Yield each item with its path in the tree.""" children = getattr(self, 'children', None) children_type = type(children).__name__ @@ -224,7 +224,7 @@ def traverse_with_paths(self): # children is just a component if isinstance(children, Component): yield "[*] " + children_string, children - for p, t in children.traverse_with_paths(): + for p, t in children._traverse_with_paths(): yield "\n".join(["[*] " + children_string, p]), t # children is a list of components @@ -238,12 +238,12 @@ def traverse_with_paths(self): yield list_path, i if isinstance(i, Component): - for p, t in i.traverse_with_paths(): + for p, t in i._traverse_with_paths(): yield "\n".join([list_path, p]), t def __iter__(self): """Yield IDs in the tree of children.""" - for t in self.traverse(): + for t in self._traverse(): if (isinstance(t, Component) and getattr(t, 'id', None) is not None): diff --git a/tests/unit/dash/development/test_base_component.py b/tests/unit/dash/development/test_base_component.py index ab1a3ce2f3..c91baa6ceb 100644 --- a/tests/unit/dash/development/test_base_component.py +++ b/tests/unit/dash/development/test_base_component.py @@ -150,7 +150,7 @@ def test_del_item_with_nested_children_with_mixed_strings_and_without_lists(self def test_traverse_with_nested_children_with_mixed_strings_and_without_lists(self): # noqa: E501 c, c1, c2, c3, c4, c5 = nested_tree() - elements = [i for i in c.traverse()] + elements = [i for i in c._traverse()] self.assertEqual( elements, c.children + [c3] + [c2] + c2.children @@ -160,7 +160,7 @@ def test_traverse_with_tuples(self): # noqa: E501 c, c1, c2, c3, c4, c5 = nested_tree() c2.children = tuple(c2.children) c.children = tuple(c.children) - elements = [i for i in c.traverse()] + elements = [i for i in c._traverse()] self.assertEqual( elements, list(c.children) + [c3] + [c2] + list(c2.children) From 6102bf0a407a02a8e9ac098144987158e8d39b95 Mon Sep 17 00:00:00 2001 From: alexcjohnson Date: Wed, 29 May 2019 16:45:32 -0400 Subject: [PATCH 3/8] more robust path to local test files in test_base_component --- .../dash/development/test_base_component.py | 23 +++++++------------ 1 file changed, 8 insertions(+), 15 deletions(-) diff --git a/tests/unit/dash/development/test_base_component.py b/tests/unit/dash/development/test_base_component.py index c91baa6ceb..7195e08261 100644 --- a/tests/unit/dash/development/test_base_component.py +++ b/tests/unit/dash/development/test_base_component.py @@ -16,6 +16,8 @@ js_to_py_type ) +_dir = os.path.dirname(os.path.abspath(__file__)) + Component._prop_names = ('id', 'a', 'children', 'style', ) Component._type = 'TestComponent' Component._namespace = 'test_namespace' @@ -487,8 +489,7 @@ def test_iter(self): class TestGenerateClassFile(unittest.TestCase): def setUp(self): - json_path = os.path.join( - 'tests', 'unit', 'dash', 'development', 'metadata_test.json') + json_path = os.path.join(_dir, 'metadata_test.json') with open(json_path) as data_file: json_string = data_file.read() data = json\ @@ -527,9 +528,7 @@ def setUp(self): self.written_class_string = f.read() # The expected result for both class string and class file generation - expected_string_path = os.path.join( - 'tests', 'unit', 'dash', 'development', 'metadata_test.py' - ) + expected_string_path = os.path.join(_dir, 'metadata_test.py') with open(expected_string_path, 'r') as f: self.expected_class_string = f.read() @@ -557,8 +556,7 @@ def test_class_file(self): class TestGenerateClass(unittest.TestCase): def setUp(self): - path = os.path.join( - 'tests', 'unit', 'dash', 'development', 'metadata_test.json') + path = os.path.join(_dir, 'metadata_test.json') with open(path) as data_file: json_string = data_file.read() data = json\ @@ -573,10 +571,7 @@ def setUp(self): namespace='TableComponents' ) - path = os.path.join( - 'tests', 'unit', 'dash', 'development', - 'metadata_required_test.json' - ) + path = os.path.join(_dir, 'metadata_required_test.json') with open(path) as data_file: json_string = data_file.read() required_data = json\ @@ -746,8 +741,7 @@ def test_required_props(self): class TestMetaDataConversions(unittest.TestCase): def setUp(self): - path = os.path.join( - 'tests', 'unit', 'dash', 'development', 'metadata_test.json') + path = os.path.join(_dir, 'metadata_test.json') with open(path) as data_file: json_string = data_file.read() data = json\ @@ -930,8 +924,7 @@ def assert_docstring(assertEqual, docstring): class TestFlowMetaDataConversions(unittest.TestCase): def setUp(self): - path = os.path.join( - 'tests', 'unit', 'dash', 'development', 'flow_metadata_test.json') + path = os.path.join(_dir, 'flow_metadata_test.json') with open(path) as data_file: json_string = data_file.read() data = json\ From e8ff4f49043b5e293b61239f3210076a520f0c28 Mon Sep 17 00:00:00 2001 From: alexcjohnson Date: Wed, 29 May 2019 17:11:32 -0400 Subject: [PATCH 4/8] blacklist the attrs we need in Py from being component props --- dash/development/component_generator.py | 21 +++++++++++-- dash/extract-meta.js | 31 ++++++++++++++++--- .../dash/development/test_base_component.py | 18 +++++++++++ 3 files changed, 63 insertions(+), 7 deletions(-) diff --git a/dash/development/component_generator.py b/dash/development/component_generator.py index 69dfafb28a..1b64dcc204 100644 --- a/dash/development/component_generator.py +++ b/dash/development/component_generator.py @@ -19,6 +19,15 @@ from ._py_components_generation import generate_classes_files +forbidden_props = [ + 'UNDEFINED', + 'REQUIRED', + 'to_plotly_json', + 'available_properties', + 'available_wildcard_properties' +] + + class _CombinedFormatter( argparse.ArgumentDefaultsHelpFormatter, argparse.RawDescriptionHelpFormatter @@ -47,9 +56,15 @@ def generate_components( extract_path = pkg_resources.resource_filename("dash", "extract-meta.js") + forbid_patterns = '|'.join( + '^{}$'.format(p) for p in forbidden_props + ['_.*'] + ) + os.environ["NODE_PATH"] = "node_modules" cmd = shlex.split( - "node {} {} {}".format(extract_path, ignore, components_source), + "node {} {} {} {}".format( + extract_path, ignore, forbid_patterns, components_source + ), posix=not is_windows, ) @@ -138,8 +153,8 @@ def cli(): "-p", "--package-info-filename", default="package.json", - help="The filename of the copied `package.json` \ -to `project_shortname`", + help="The filename of the copied `package.json` " + "to `project_shortname`", ) parser.add_argument( "-i", diff --git a/dash/extract-meta.js b/dash/extract-meta.js index b667a58adc..4a6bdc8ac7 100644 --- a/dash/extract-meta.js +++ b/dash/extract-meta.js @@ -4,8 +4,11 @@ const fs = require('fs'); const path = require('path'); const reactDocs = require('react-docgen'); -const componentPaths = process.argv.slice(3); +const componentPaths = process.argv.slice(4); const ignorePattern = new RegExp(process.argv[2]); +const forbiddenProps = process.argv[3].split('|').map(part => new RegExp(part)); + +let failed = false; const excludedDocProps = [ 'setProps', 'id', 'className', 'style' @@ -20,13 +23,18 @@ const metadata = Object.create(null); componentPaths.forEach(componentPath => collectMetadataRecursively(componentPath) ); -writeOut(metadata); +if (failed) { + console.error('\nextract-meta failed.\n'); +} +else { + writeOut(metadata); +} function help() { console.error('usage: '); console.error( - 'extract-meta path/to/component(s) ' + - ' [path/to/more/component(s), ...] > metadata.json' + 'extract-meta ^fileIgnorePattern ^forbidden$|^props$|^patterns$' + + ' path/to/component(s) [path/to/more/component(s) ...] > metadata.json' ); } @@ -55,6 +63,20 @@ function docstringWarning(doc) { ); } +function propError(doc) { + for(const propName in doc.props) { + forbiddenProps.forEach(forbiddenPattern => { + if (forbiddenPattern.test(propName)) { + process.stderr.write( + `\nERROR: "${propName}" matches forbidden prop name ` + + `pattern: ${forbiddenPattern.toString()}\n` + ); + failed = true; + } + }); + } +} + function parseFile(filepath) { const urlpath = filepath.split(path.sep).join('/'); @@ -68,6 +90,7 @@ function parseFile(filepath) { src = fs.readFileSync(filepath); const doc = metadata[urlpath] = reactDocs.parse(src); docstringWarning(doc); + propError(doc); } catch (error) { writeError(error, filepath); } diff --git a/tests/unit/dash/development/test_base_component.py b/tests/unit/dash/development/test_base_component.py index 7195e08261..f1ab4e261f 100644 --- a/tests/unit/dash/development/test_base_component.py +++ b/tests/unit/dash/development/test_base_component.py @@ -7,6 +7,7 @@ import plotly from dash.development.base_component import Component +from dash.development.component_generator import forbidden_props from dash.development._py_components_generation import ( generate_class_string, generate_class_file, @@ -738,6 +739,23 @@ def test_required_props(self): with self.assertRaises(Exception): self.ComponentClassRequired(children='test') + def test_attrs_match_forbidden_props(self): + # props are not added as attrs unless explicitly provided + # except for children, which is always set if it's a prop at all. + c = self.ComponentClass() + base_attrs = dir(c) + extra_attrs = [a for a in base_attrs if a[0] != '_'] + self.assertEqual(set(extra_attrs), set(forbidden_props + ['children'])) + + # setting props causes them to show up as attrs + c2 = self.ComponentClass('children', id='c2', optionalArray=[1]) + prop_attrs = dir(c2) + self.assertEqual(set(base_attrs) - set(prop_attrs), set([])) + self.assertEqual( + set(prop_attrs) - set(base_attrs), + set(['id', 'optionalArray']) + ) + class TestMetaDataConversions(unittest.TestCase): def setUp(self): From 843223978f2179dab3b0226f5abb35babca191c9 Mon Sep 17 00:00:00 2001 From: alexcjohnson Date: Wed, 29 May 2019 20:48:12 -0400 Subject: [PATCH 5/8] allow access of "protected" _traverse methods --- dash/dash.py | 2 ++ dash/development/base_component.py | 2 ++ 2 files changed, 4 insertions(+) diff --git a/dash/dash.py b/dash/dash.py index 9f5feb0551..292d8f207b 100644 --- a/dash/dash.py +++ b/dash/dash.py @@ -914,6 +914,7 @@ def _value_is_valid(val): def _validate_value(val, index=None): # val is a Component if isinstance(val, Component): + # pylint: disable=protected-access for p, j in val._traverse_with_paths(): # check each component value in the tree if not _value_is_valid(j): @@ -1194,6 +1195,7 @@ def _validate_layout(self): layout_id = getattr(self.layout, 'id', None) component_ids = {layout_id} if layout_id else set() + # pylint: disable=protected-access for component in to_validate._traverse(): component_id = getattr(component, 'id', None) if component_id and component_id in component_ids: diff --git a/dash/development/base_component.py b/dash/development/base_component.py index cfd829f61b..79e7110c0c 100644 --- a/dash/development/base_component.py +++ b/dash/development/base_component.py @@ -224,6 +224,7 @@ def _traverse_with_paths(self): # children is just a component if isinstance(children, Component): yield "[*] " + children_string, children + # pylint: disable=protected-access for p, t in children._traverse_with_paths(): yield "\n".join(["[*] " + children_string, p]), t @@ -238,6 +239,7 @@ def _traverse_with_paths(self): yield list_path, i if isinstance(i, Component): + # pylint: disable=protected-access for p, t in i._traverse_with_paths(): yield "\n".join([list_path, p]), t From f0cec2fb27ec43df1c030b9df82b7519f45e4e9b Mon Sep 17 00:00:00 2001 From: alexcjohnson Date: Wed, 29 May 2019 21:26:44 -0400 Subject: [PATCH 6/8] changelog for props-blacklist --- dash/CHANGELOG.md | 2 ++ 1 file changed, 2 insertions(+) diff --git a/dash/CHANGELOG.md b/dash/CHANGELOG.md index ad5d1a350c..ef8b9027a4 100644 --- a/dash/CHANGELOG.md +++ b/dash/CHANGELOG.md @@ -1,5 +1,7 @@ ## Unreleased ### Changed +- [#753](https://github.com/plotly/dash/pull/753) `Component` no longer inherits `MutableMapping`, so `values`, `keys`, and more are no longer methods. This fixed an issue reported in [dcc](https://github.com/plotly/dash-core-components/issues/440) where components with certain prop names defined but not provided would cause a failure to render. During component generation we now disallow all props with leading underscores or matching a few remaining reserved words: `UNDEFINED`, `REQUIRED`, `to_plotly_json`, `available_properties`, and `available_wildcard_properties`. + - [#739](https://github.com/plotly/dash/pull/739) Allow the Flask app to be provided to Dash after object initialization. This allows users to define Dash layouts etc when using the app factory pattern, or any other pattern that inhibits access to the app object. This broadly complies with the flask extension API, allowing Dash to be considered as a Flask extension where it needs to be. - [#722](https://github.com/plotly/dash/pull/722) Assets are served locally by default. Both JS scripts and CSS files are affected. This improves robustness and flexibility in numerous situations, but in certain cases initial loading could be slowed. To restore the previous CDN serving, set `app.scripts.config.serve_locally = False` (and similarly with `app.css`, but this is generally less important). From e813a1fbb9b0431db9512a60a4630bf19fac853c Mon Sep 17 00:00:00 2001 From: alexcjohnson Date: Thu, 30 May 2019 12:18:36 -0400 Subject: [PATCH 7/8] use assert in new prop blacklist tests --- .../dash/development/test_base_component.py | 23 ++++++++++--------- 1 file changed, 12 insertions(+), 11 deletions(-) diff --git a/tests/unit/dash/development/test_base_component.py b/tests/unit/dash/development/test_base_component.py index f1ab4e261f..171ee3f3e4 100644 --- a/tests/unit/dash/development/test_base_component.py +++ b/tests/unit/dash/development/test_base_component.py @@ -469,23 +469,23 @@ def test_iter(self): 'setdefault', 'update', 'values'] for m in mixins: - self.assertFalse(hasattr(c, m), m) + assert not hasattr(c, m), 'should not have method ' + m keys = ['2', '3', '4', '5', '6', '7', '8'] for k in keys: # test __contains__() - self.assertTrue(k in c) + assert k in c, 'should find key ' + k # test __getitem__() - self.assertEqual(c[k].id, k) + assert c[k].id == k, 'key {} points to the right item'.format(k) # test __iter__() keys2 = [] for k in c: keys2.append(k) - self.assertIn(k, keys) + assert k in keys, 'iteration produces key ' + k - self.assertEqual(len(keys), len(keys2)) + assert len(keys) == len(keys2), 'iteration produces no extra keys' class TestGenerateClassFile(unittest.TestCase): @@ -745,16 +745,17 @@ def test_attrs_match_forbidden_props(self): c = self.ComponentClass() base_attrs = dir(c) extra_attrs = [a for a in base_attrs if a[0] != '_'] - self.assertEqual(set(extra_attrs), set(forbidden_props + ['children'])) + assert set(extra_attrs) == set(forbidden_props + ['children']), \ + 'component has only underscored and reserved word attrs' # setting props causes them to show up as attrs c2 = self.ComponentClass('children', id='c2', optionalArray=[1]) prop_attrs = dir(c2) - self.assertEqual(set(base_attrs) - set(prop_attrs), set([])) - self.assertEqual( - set(prop_attrs) - set(base_attrs), - set(['id', 'optionalArray']) - ) + assert set(base_attrs) - set(prop_attrs) == set([]), \ + 'no attrs were removed' + assert ( + set(prop_attrs) - set(base_attrs) == set(['id', 'optionalArray']) + ), 'explicit props were added as attrs' class TestMetaDataConversions(unittest.TestCase): From 64388b4e76c90b74cbda69cc394cab30e71ad527 Mon Sep 17 00:00:00 2001 From: alexcjohnson Date: Thu, 30 May 2019 12:44:30 -0400 Subject: [PATCH 8/8] forbidden_props -> reserved_words, and include `_.*` --- dash/development/component_generator.py | 11 +++++----- dash/extract-meta.js | 10 +++++----- .../dash/development/test_base_component.py | 20 +++++++++++-------- 3 files changed, 22 insertions(+), 19 deletions(-) diff --git a/dash/development/component_generator.py b/dash/development/component_generator.py index 1b64dcc204..48b88c0928 100644 --- a/dash/development/component_generator.py +++ b/dash/development/component_generator.py @@ -19,12 +19,13 @@ from ._py_components_generation import generate_classes_files -forbidden_props = [ +reserved_words = [ 'UNDEFINED', 'REQUIRED', 'to_plotly_json', 'available_properties', - 'available_wildcard_properties' + 'available_wildcard_properties', + '_.*' ] @@ -56,14 +57,12 @@ def generate_components( extract_path = pkg_resources.resource_filename("dash", "extract-meta.js") - forbid_patterns = '|'.join( - '^{}$'.format(p) for p in forbidden_props + ['_.*'] - ) + reserved_patterns = '|'.join('^{}$'.format(p) for p in reserved_words) os.environ["NODE_PATH"] = "node_modules" cmd = shlex.split( "node {} {} {} {}".format( - extract_path, ignore, forbid_patterns, components_source + extract_path, ignore, reserved_patterns, components_source ), posix=not is_windows, ) diff --git a/dash/extract-meta.js b/dash/extract-meta.js index 4a6bdc8ac7..3567161bee 100644 --- a/dash/extract-meta.js +++ b/dash/extract-meta.js @@ -6,7 +6,7 @@ const reactDocs = require('react-docgen'); const componentPaths = process.argv.slice(4); const ignorePattern = new RegExp(process.argv[2]); -const forbiddenProps = process.argv[3].split('|').map(part => new RegExp(part)); +const reservedPatterns = process.argv[3].split('|').map(part => new RegExp(part)); let failed = false; @@ -65,11 +65,11 @@ function docstringWarning(doc) { function propError(doc) { for(const propName in doc.props) { - forbiddenProps.forEach(forbiddenPattern => { - if (forbiddenPattern.test(propName)) { + reservedPatterns.forEach(reservedPattern => { + if (reservedPattern.test(propName)) { process.stderr.write( - `\nERROR: "${propName}" matches forbidden prop name ` + - `pattern: ${forbiddenPattern.toString()}\n` + `\nERROR: "${propName}" matches reserved word ` + + `pattern: ${reservedPattern.toString()}\n` ); failed = true; } diff --git a/tests/unit/dash/development/test_base_component.py b/tests/unit/dash/development/test_base_component.py index 171ee3f3e4..739590ccf7 100644 --- a/tests/unit/dash/development/test_base_component.py +++ b/tests/unit/dash/development/test_base_component.py @@ -7,7 +7,7 @@ import plotly from dash.development.base_component import Component -from dash.development.component_generator import forbidden_props +from dash.development.component_generator import reserved_words from dash.development._py_components_generation import ( generate_class_string, generate_class_file, @@ -740,21 +740,25 @@ def test_required_props(self): self.ComponentClassRequired(children='test') def test_attrs_match_forbidden_props(self): + assert '_.*' in reserved_words, 'props cannot have leading underscores' + # props are not added as attrs unless explicitly provided # except for children, which is always set if it's a prop at all. + expected_attrs = set(reserved_words + ['children']) - set(['_.*']) c = self.ComponentClass() - base_attrs = dir(c) - extra_attrs = [a for a in base_attrs if a[0] != '_'] - assert set(extra_attrs) == set(forbidden_props + ['children']), \ + base_attrs = set(dir(c)) + extra_attrs = set(a for a in base_attrs if a[0] != '_') + + assert extra_attrs == expected_attrs, \ 'component has only underscored and reserved word attrs' # setting props causes them to show up as attrs c2 = self.ComponentClass('children', id='c2', optionalArray=[1]) - prop_attrs = dir(c2) - assert set(base_attrs) - set(prop_attrs) == set([]), \ - 'no attrs were removed' + prop_attrs = set(dir(c2)) + + assert base_attrs - prop_attrs == set([]), 'no attrs were removed' assert ( - set(prop_attrs) - set(base_attrs) == set(['id', 'optionalArray']) + prop_attrs - base_attrs == set(['id', 'optionalArray']) ), 'explicit props were added as attrs'