search

gap-packages / JupyterKernel

Native Jupyter kernel for GAP
https://gap-packages.github.io/JupyterKernel/
BSD 3-Clause "New" or "Revised" License
19 stars 12 forks source link

Clean up setup.py #106

Closed isuruf closed 3 years ago

isuruf commented 4 years ago

You can run this script as pip install . or pip install --user

codecov[bot] commented 4 years ago

Codecov Report

Merging #106 into master will decrease coverage by 29.01%. The diff coverage is n/a.

@@             Coverage Diff             @@
##           master     #106       +/-   ##
===========================================
- Coverage    71.9%   42.89%   -29.02%     
===========================================
  Files          14       14               
  Lines         865      865               
===========================================
- Hits          622      371      -251     
- Misses        243      494      +251
Impacted Files Coverage Δ
gap/JupyterKernel.gi 35.58% <0%> (-44.57%) :arrow_down:
gap/JupyterHelp.gi 8.64% <0%> (-39.46%) :arrow_down:
gap/JupyterMsg.gi 53.93% <0%> (-37.08%) :arrow_down:
gap/JupyterRenderable.gi 70.83% <0%> (-29.17%) :arrow_down:
gap/JupyterInspection.gi 71.83% <0%> (-15.5%) :arrow_down:
gap/JupyterUtil.gi 28.23% <0%> (-8.24%) :arrow_down:
gap/JupyterStream.gi 68.57% <0%> (-1.43%) :arrow_down:
codecov[bot] commented 4 years ago

Codecov Report

Merging #106 into master will not change coverage. The diff coverage is n/a.

@@          Coverage Diff           @@
##           master    #106   +/-   ##
======================================
  Coverage    71.9%   71.9%           
======================================
  Files          14      14           
  Lines         865     865           
======================================
  Hits          622     622           
  Misses        243     243
olexandr-konovalov commented 4 years ago

Thanks @isuruf! Could you please explain the purpose of gap-mode.json and also of the Binder addition?

isuruf commented 4 years ago

gap-mode.json is there so that the gap nbextension is enabled automatically with notebook>=5.3. No need to do enable the extension after installing anymore.

Binder lets users try the jupyter notebook on cloud. See https://mybinder.org/v2/gh/isuruf/GapJupyterKernel/binder?filepath=demos%2Fgraph-visjs.ipynb

isuruf commented 4 years ago

These changes also let you publish this package in PyPI.

olexandr-konovalov commented 4 years ago

I use binder, see e.g. https://github.com/alex-konovalov/gap-teaching - I mean, what's the benefit of adding the files to support binder into the package?

isuruf commented 4 years ago

Adding it here means, people can try out new features in this package. I can remove it if you want.

olexandr-konovalov commented 4 years ago

I would prefer to add Binder support in a separate PR, and also would have questions about its current proposal. By the way, we have https://github.com/gap-system/try-gap-in-jupyter where users may try it, and it would be good to add a binder badge to the README in this repo, but I'd prefer to use gapsystem/gap-docker from Docker Hub to run it.

codecov-io commented 4 years ago

Codecov Report

Merging #106 into master will not change coverage. The diff coverage is n/a.

@@          Coverage Diff           @@
##           master    #106   +/-   ##
======================================
  Coverage    71.9%   71.9%           
======================================
  Files          14      14           
  Lines         865     865           
======================================
  Hits          622     622           
  Misses        243     243
olexandr-konovalov commented 4 years ago

It will be useful to describe why egg needs to be disabled. This package has no fixed maintainer at the moment, so please help us to understand this PR by giving some more details.

isuruf commented 4 years ago

Sure. The data_files argument with etc or share directories doesn't work when building an egg. You need to use pip install . which creates a wheel first and then installs the package. This is what other nbextension packages like nbconvert, pari_jupyter does.

See https://github.com/jupyter/nbconvert/blob/master/setup.py and https://github.com/jdemeyer/pari_jupyter/blob/master/setup.py

olexandr-konovalov commented 4 years ago

Thanks @isuruf. Does this change any installation instructions? Do we need to test this manually, or the fact that .travis.yml changes result in passing tests should suffice?

isuruf commented 4 years ago

Does this change any installation instructions?

Yes. You need to do pip install . instead of python setup.py install

olexandr-konovalov commented 4 years ago

In which case, could you please also change instructions in https://github.com/gap-packages/JupyterKernel/blob/master/doc/intro.xml ?

olexandr-konovalov commented 3 years ago

@isuruf sorry, this was approved long time ago but not merged. Would you mind to rebase it to test again? In the meantime, we switched from Travis to GitHub actions, so that may need an update.

olexandr-konovalov commented 3 years ago

I've tested it locally, seems working, so I've dropped the commit changing .travis.yml and pushed the rest in 4 commits to the master: c671159157d80e57b90bea5df916284b03930890, a8ce174df5f7b595090cd46d7dc08f59d0c2c1ab, ab45c2ea2b609fb85078e364981098359e57a416 and dad85566bdfc52aef19eef8db70b0d6702b24151. CI tests pass, although they do not install the Jupyter part at all - they only test the GAP side of things, but we should be testing this in various other settings from now on.

olexandr-konovalov commented 3 years ago

These changes also let you publish this package in PyPI.

@isuruf I am not sure we need it - what would be the hypothetical use of pip3 install jupyter-kernel-gap if there is no GAP on the user's machine? And if there is GAP with packages, then this package is already in pkg subdirectory of their GAP installation. Or do I miss something?