-
Notifications
You must be signed in to change notification settings - Fork 55
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
Update for new registerWidget syntax #3
Conversation
@blink1073 - just confirming - is this now ready to go in from your perspective? |
Actually I just finished the |
Cool, thanks |
Boom, this and jupyter/jupyter-packaging#19 are gtg. |
Tested this out in the classic notebook, and the |
I'm creating a new cookiecutter output with this branch, then doing |
Building an sdist |
It appears that the problem is that the get_data_files at https://github.com/blink1073/opinionated-widget-cookiecutter/blob/4e6a4275370e422e30ec9ed22d4c6cc2fd7bf76a/%7B%7Bcookiecutter.github_project_name%7D%7D/setup.py#L53 runs before npm has run its course generating the files, so only the extension.js file is picked up, not the generated index.js file. |
I don't run into this with ipywidgets because I explicitly run the js build step before the packaging step (i.e., we don't have python run the js build step). It does seem that there is a difference here between building a wheel vs building an sdist. |
nb_path = os.path.join(here, name, 'nbextension', 'static') | ||
lab_path = os.path.join(here, name, 'labextension', '*.tgz') | ||
nb_path = os.path.join(HERE, name, 'nbextension', 'static') | ||
lab_path = os.path.join(HERE, name, 'labextension', '*.tgz') | ||
|
||
# Representative files that should exist after a successful build | ||
jstargets = [ | ||
os.path.join(nb_path, 'extension.js'), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
index.js
is generated, not extension.js
@blink1073 - thanks! Did you not use the glob module because it only supports |
Also I noticed that a bunch of the placeholders (like |
Yes, ideally we'd all be using Py35+ 😄. I had attempted to get |
I recently made globmatch. Maybe it can be useful while waiting for the future? |
Oh right, that will be a general problem for any non-standard package I guess. |
I'll make sure that the glob behavior gives the intended files in Jupyter Lab as well. |
Okay, in c2b50d2 I updated the package data to not include the root (they are relative file paths from the package root), and ensured that |
I reinstated |
Note that the In [7]: glob.glob('./jupyterlab/staging/*', recursive=True)
Out[7]:
['./jupyterlab/staging/node_modules',
'./jupyterlab/staging/index.js',
'./jupyterlab/staging/webpack.config.js',
'./jupyterlab/staging/yarn.lock',
'./jupyterlab/staging/package.json',
'./jupyterlab/staging/yarn.js',
'./jupyterlab/staging/build']
In [8]: glob.glob('./jupyterlab/staging/**', recursive=True)
Out[8]:
['./jupyterlab/staging/',
'./jupyterlab/staging/index.js',
'./jupyterlab/staging/webpack.config.js',
'./jupyterlab/staging/yarn.lock',
'./jupyterlab/staging/package.json',
'./jupyterlab/staging/yarn.js',
'./jupyterlab/staging/build',
'./jupyterlab/staging/build/index.out.js',
'./jupyterlab/staging/build/af7ae505a9eed503f8b8e6982036873e.woff2',
'./jupyterlab/staging/build/674f50d287a8c48dc19ba404d20fe713.eot',
'./jupyterlab/staging/build/main.bundle.js.map',
'./jupyterlab/staging/build/0.bundle.js.map',
'./jupyterlab/staging/build/b06871f281fee6b241d60582ae9369b9.ttf',
'./jupyterlab/staging/build/package.json',
'./jupyterlab/staging/build/0.bundle.js',
'./jupyterlab/staging/build/912ec66d7572ff821749319396470bde.svg',
'./jupyterlab/staging/build/fee66e712a8a08eef5805a46892932ad.woff',
'./jupyterlab/staging/build/main.bundle.js'] |
While I agree with the purpose of the glob change, it breaks the following patterns:
I'll iterate on the algorithm on Monday to see if I can include the wanted behavior. |
I would say match the behavior of the stdlib so we don't get confusion when we can switch later. If you want to recurse into a directory, you must use In [3]: glob.glob('./jupyterlab/staging/', recursive=True)
Out[3]: ['./jupyterlab/staging/'] |
(at least in this location, you are free to be more opinionated in |
BTW: I see that setuptools excludes data files from package data. That's probably something that we should do as well. |
That behavior is still quite strange. Try |
I did push changes to the globmatch repo that achieves this, but holding back on pushing it here. |
Okay, rather than try and meet the 3.5 spec entirely, I'd say let's inline |
I'm flip-flopping a little here: I was considering the case where you have a
|
I'd say that |
Ensure we do not recurse for directory name matches, except when ending on **.
Pushed changes I think I'm okay with: Any directory match basically does nothing. Add an explicit trailing |
PS: For globmatch I put the behavior behind a flag. |
Tested current version of pythreejs and it seems to work well (need to add a custom step to update packages since
|
I don't think we should use |
I thought that the Regarding Considering that, I'll merge for now and transfer the repo. |
Sweet! Pleasure doing business with you 😄. |
Also wip changes to
setupbase.py
that were coinciding rewriting JupyterLab'ssetupbase.py
. I can submit a PR here andjupyter-packaging
with the final version.