Combine extract-meta and component generation in a cli. #451

T4rk1n · 2018-11-06T22:19:18Z

Add extract-meta.js
Combine extract-meta.js call and component generation.
Wrap as a cli under the command dash-generate-components

Usage:
$ dash-generate-components src/lib/components my_components_namespace

cc @plotly/dash

Closes plotly/dash-component-boilerplate#17

nicolaskruchten · 2018-11-07T03:39:24Z

This seems like a good step forward. A few questions:

Is this backwards-compatible with existing components written using the boilerplate?
Can we see the matching PR in the boilerplate repo that calls this new script?
Should we wait until WIP Initial version of updated code for R transpiler #449 is merged and refactor the two together, perhaps with an --experimental-r-components flag?

T4rk1n · 2018-11-07T20:33:27Z

Is this backwards-compatible with existing components written using the boilerplate?

Yes, the old code was not modified.

Can we see the matching PR in the boilerplate repo that calls this new script?

Coming up soon.

Should we wait until #449 is merged and refactor the two together, perhaps with an --experimental-r-components flag?

Just need to add generate_class_file_r call in the loop next to generate_class_file. Should be easy to refactor, can add the argument with argparse.

nicolaskruchten · 2018-11-07T21:01:20Z

dash/development/component_generator.py

+
+
+# pylint: disable=too-many-locals
+def generate_components(component_src, output_dir):


I would rename output_dir to project_shortname and not treat it as a path.

nicolaskruchten · 2018-11-07T21:01:33Z

dash/development/component_generator.py

+    cmd = shlex.split('node {} {}'.format(extract_path, component_src),
+                      posix=not is_windows)
+
+    namespace = os.path.basename(output_dir)


I would just remove namespace as a variable here.

nicolaskruchten · 2018-11-07T21:02:46Z

dash/development/component_generator.py

+        )
+        sys.exit(1)
+    # pylint: disable=unbalanced-tuple-unpacking
+    src, out = sys.argv[1:]


again, I would rename this project_shortname instead of out... because the R code won't output there.

nicolaskruchten · 2018-11-07T21:07:19Z

dash/development/component_generator.py

+            component_data['description'],
+            namespace
+        )
+        print('Generated {}/{}.py'.format(namespace, name))


for the Python piece, if it turns out that we do need package.json in the Python module directory next to __init__.py then I would do the copying here

See also plotly/dash-component-boilerplate#38

nicolaskruchten · 2018-11-07T21:09:19Z

💃 once the tests pass and the internal logic of the program are little bit more language-agnostic :)

nicolaskruchten · 2018-11-13T14:57:04Z

dash/development/component_generator.py

+            component_data['description'],
+            project_shortname
+        )
+        print('Generated {}/{}.py'.format(project_shortname, name))


can we move this line into the generate_class_file function? It's Python-specific and there will likely be an R-specific version as well and i don't think it's good to duplicate the path-as-function-of-project_shortname logic here as well :)

i don't think it's good to duplicate the path-as-function-of-project_shortname logic here as well

I don't understand what you mean by that, just print the component name ?

I'm saying that in the case of Python, the file path happens to be project_shortname/name`` but for R it'll be camelCase(project_shortname)/name.R` and I don't want to duplicate that logic here...

We could just print Generated {lang} version of {component_name} instead and bypass the language-specific path details :)

nicolaskruchten · 2018-11-13T14:57:38Z

dash/development/component_generator.py

+    components = []
+
+    for component_path, component_data in metadata.items():
+        name = component_path.split('/')[-1].split('.')[0]


maybe name this component_name just for extra clarity/readability

nicolaskruchten · 2018-11-13T14:58:23Z

dash/development/component_generator.py

+    with open(os.path.join(project_shortname, 'metadata.json'), 'w') as f:
+        json.dump(metadata, f)
+
+    with open(os.path.join(project_shortname, '_imports_.py'), 'w') as f:


can we move this Python-specific stuff into a Python-specific place? doesn't have to be in this PR but just in general :)

It's actually duplicate code so let's refactor that for the old method too.

nicolaskruchten · 2018-11-13T19:09:24Z

dash/development/component_generator.py


+from dash.development.component_loader import generate_imports


should this also be a relative import? :)

nicolaskruchten · 2018-11-13T20:09:24Z

dash/development/component_generator.py

+
+    components = []
+
+    for component_path, component_data in metadata.items():


as a general comment, this seems to be repeated code with

dash/dash/development/component_loader.py

Line 68 in d892b9e

name = componentPath.split('/').pop().split('.')[0]

... any way to DRY it up?

T4rk1n · 2018-11-14T21:50:06Z

@nicolaskruchten Tests are in plotly/dash-component-boilerplate#40

@rmarren1 Please review

rmarren1

This is great! I think we should fully commit to one method of generating class files.

With this change it looks like you can either

Run the extract-meta.js command manually to get metadata.json
Run dash.development.component_loader.generate_classes pointing to the new metadata.json
OR
Run dash-generate-components to do all that in one function.

So we can either

In generate_components, we can write metadata.json first, then use a call to dash.development.component_loader.generate_classes from within generate_components to generate the components.
Just delete the dash.development.component_loader.generate_classes function and rely on the CLI.

I think 2 might cause some minor headaches since we would need to update the toolchains in dash-*-components to use the CLI rather than dash.development.component_loader.generate_classes, but also dash.development.component_loader.generate_classes isn't really doing much. I'm pro deleting un-needed code, so I would go with #2 unless there is something I am not thinking of.

Also, I think base_component is getting a bit busy, I would be in favor of moving all the I/O functions to component_generator.py, e.g. generate_class_file, generate_imports, generate_classes_files.

Other than the method to get the path to extract-meta.js is the only critical change here.

rmarren1 · 2018-11-26T16:39:13Z

dash/development/base_component.py

@@ -4,6 +4,8 @@
 import inspect
 import abc
 import sys
+import textwrap


rmarren1 · 2018-11-26T16:39:46Z

dash/development/base_component.py

@@ -466,6 +468,8 @@ def generate_class_file(typename, props, description, namespace):
        f.write(import_string)
        f.write(class_string)

+    print('Generated {}'.format(file_name))


rmarren1 · 2018-11-26T18:18:47Z

dash/development/base_component.py

+        ).lstrip())
+
+
+def generate_classes_files(project_shortname, metadata, *component_generators):


Can this be generate_class_files to match generate_class and generate_class_file method names?

Also, can it be moved up to be next to those methods? Just to keep them together spatially in the code

Actually, perhaps we can just move these into components_generator.py

rmarren1 · 2018-11-26T18:32:17Z

dash/development/component_generator.py

+                        package_info_filename='package.json'):
+    is_windows = sys.platform == 'win32'
+
+    extract_path = os.path.abspath(os.path.join(


This pattern for accessing the included files has given me problems in the past, from what I researched to fix it we should use https://stackoverflow.com/a/20885799

rmarren1 · 2018-11-26T18:37:27Z

dash/development/component_generator.py

+    ))
+
+    os.environ['NODE_PATH'] = 'node_modules'
+    cmd = shlex.split('node {} {}'.format(extract_path, components_source),


Is this different from ['node', extract_path, component_source]? Never used shlex not sure what this does.

I use shlex to make sure it works across platforms, had problems on windows passing arguments without the posix=False argument.

Sounds good 👍

rmarren1 · 2018-11-26T19:10:32Z

dash/development/component_loader.py

-        # Add an import statement for this component
-        with open(imports_path, 'a') as f:
-            f.write('from .{0:s} import {0:s}\n'.format(name))
+    components = generate_classes_files(namespace, data, generate_class_file)


Four lines up is not relevant anymore, write append not used.

nicolaskruchten · 2018-11-26T19:48:40Z

@rmarren1 I think we need to keep supporting both methods for backwards-compatibility, otherwise older components may break, no?

T4rk1n · 2018-11-26T19:49:06Z

In generate_components, we can write metadata.json first, then use a call to dash.development.component_loader.generate_classes from within generate_components to generate the components.

Writing just to read back seems like a bad pattern, I initially did not write the metadata.json and just generate the components but figured it should still be there for backward compatibility.

Just delete the dash.development.component_loader.generate_classes function and rely on the CLI.

I think they should stay there for backward compatibility. Someone working on a component library should be able to update dash without breaking their build system.

T4rk1n · 2018-11-26T22:33:16Z

@rmarren1

I moved all python class generation code to _py_components_generation, only kept the component, meta and registry in base_component. component_generator should be only code needed for the cli generation. So we'll add the code for R generation in _r_components_generation and use it in component_generator cli under a experimental flag at first. Maybe I should change the name of the cli command to dash-component-generator instead of dash-generate-components to match the name of the module ?

rmarren1 · 2018-11-29T14:56:36Z

💃
I like this organizational change, should help to keep our python / r code separate and keep the base_component file slim.

Deleting the non-cli method of generating component class files would only break build systems for component authors and shouldn't affect things for end users (as long as we keep the load_components method), and this should require a one line change to build:py. I didn't think this was a 'breaking change' since it didn't affect end users, but I agree developers should be able to upgrade without having their build systems break and now am in favor of keeping it. We can just make a note of this and remove it in a future breaking release.

nicolaskruchten · 2018-11-29T15:11:00Z

@rmarren1 FWIW here's the terminology I use when thinking about our stakeholders:

Component Developers: makers of components, users of boilerplate and component-generation.
App Developers: makers of Dash apps, installers of components, users of Dash API
End Users: users of Dash apps

In general our commitment should be not to break things for any of these folks :)

nicolaskruchten · 2018-11-29T15:14:14Z

@T4rk1n I'm a bit confused about what the component_loader's responsibility here is/when it's invoked... can you enlighten me?

T4rk1n · 2018-11-29T15:25:39Z

@nicolaskruchten The component_loader is the old method for generating the components, it contains the methods for generating components at runtime and the method we were using in build:py before this PR. Keeping it for backward compatibility.

rmarren1 · 2018-11-29T15:28:48Z

Some history:

In the first dash iteration, component libraries would call component_loader.load_components in their __init__.py file the path to their metadata.json file. This code would generate component class strings and call exec to load them at runtime.

#276 changed this, so that that component classes were generated once at build time using component_loader.generate_components and imported properly into __init__.py.

Now this CLI will replace the component_loader.generate_components code, so everything there is for backward compatibility.

nicolaskruchten · 2018-11-29T15:29:27Z

thanks for the clarifications!

💃 on my end

T4rk1n requested review from nicolaskruchten and rpkyle November 7, 2018 00:23

rmarren1 self-requested a review November 7, 2018 01:22

T4rk1n force-pushed the extract-meta branch from f04bca0 to 9d758ea Compare November 7, 2018 03:06

nicolaskruchten reviewed Nov 7, 2018

View reviewed changes

nicolaskruchten mentioned this pull request Nov 7, 2018

Remove duplicate package.json, update readme. plotly/dash-component-boilerplate#33

Closed

T4rk1n mentioned this pull request Nov 7, 2018

Unable to run Cookiecutter without Python 2 plotly/dash-component-boilerplate#23

Closed

T4rk1n force-pushed the extract-meta branch 2 times, most recently from 04f7761 to 82e22c3 Compare November 13, 2018 00:25

nicolaskruchten reviewed Nov 13, 2018

View reviewed changes

T4rk1n force-pushed the extract-meta branch from 19dfa76 to e0069a9 Compare November 13, 2018 18:41

nicolaskruchten reviewed Nov 13, 2018

View reviewed changes

T4rk1n added 8 commits November 20, 2018 16:13

Add extract-meta.js.

d542118

Combine extract-meta & component generation, wrap as cli.

9863b9d

Fix setup line len

0c0d755

Add arguments validation.

1821b28

Add extract-meta to manifest.

fde8d6c

Print extract-meta errors to stderr

087a63f

Dump metadata.json

1e5adce

Add _imports_.py generation.

726623d

T4rk1n added 4 commits November 20, 2018 16:13

Disable superflous-parens pylint.

474f995

relative import.

414ac62

🐫 generate classes files loop.

25a8e94

🐾 Move generate imports/files methods to base_components.

c642b84

T4rk1n force-pushed the extract-meta branch from bf2c991 to cbaa6a5 Compare November 20, 2018 21:14

Proper cli with argparse.

b53f54a

T4rk1n force-pushed the extract-meta branch from cbaa6a5 to b53f54a Compare November 20, 2018 21:18

T4rk1n added 2 commits November 21, 2018 12:10

Copy package.json, take the output filename as option.

a3d5ae2

📛 src -> components_sources

52a56f9

rmarren1 suggested changes Nov 26, 2018

View reviewed changes

Use pkg_resources to make the extract-meta.js path.

6d8e6b7

T4rk1n force-pushed the extract-meta branch from 93ab5e6 to 5c0688c Compare November 26, 2018 22:04

🐾 Move component generation code to _py_components_generation.py

4878245

T4rk1n force-pushed the extract-meta branch from 5c0688c to 4878245 Compare November 26, 2018 22:08

Update version and changelog.

a492448

T4rk1n mentioned this pull request Nov 26, 2018

Dash 1.0.0 Series - Proposed Set of *Breaking Changes* #469

Closed

24 tasks

rmarren1 approved these changes Nov 29, 2018

View reviewed changes

T4rk1n merged commit 951c890 into master Nov 29, 2018

T4rk1n deleted the extract-meta branch November 29, 2018 16:45

HammadTheOne pushed a commit to HammadTheOne/dash that referenced this pull request May 28, 2021

Update all dependencies (plotly#451)

22bb3cc

HammadTheOne pushed a commit that referenced this pull request Jul 23, 2021

Update all dependencies (#451)

5c6614b



		# pylint: disable=too-many-locals
		def generate_components(component_src, output_dir):


		from dash.development.component_loader import generate_imports


		components = []

		for component_path, component_data in metadata.items():

		).lstrip())


		def generate_classes_files(project_shortname, metadata, *component_generators):

Combine extract-meta and component generation in a cli. #451

Combine extract-meta and component generation in a cli. #451

Conversation

T4rk1n commented Nov 6, 2018 • edited Loading

nicolaskruchten commented Nov 7, 2018

T4rk1n commented Nov 7, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nicolaskruchten commented Nov 7, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

T4rk1n commented Nov 14, 2018

rmarren1 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nicolaskruchten commented Nov 26, 2018

T4rk1n commented Nov 26, 2018

T4rk1n commented Nov 26, 2018

rmarren1 commented Nov 29, 2018 • edited Loading

nicolaskruchten commented Nov 29, 2018

nicolaskruchten commented Nov 29, 2018

T4rk1n commented Nov 29, 2018

rmarren1 commented Nov 29, 2018

nicolaskruchten commented Nov 29, 2018

T4rk1n commented Nov 6, 2018 •

edited

Loading

rmarren1 commented Nov 29, 2018 •

edited

Loading