Opened 3 years ago

Closed 2 years ago

Last modified 20 months ago

#23415 closed enhancement (fixed)

Remove sage-rst2ipynb

Reported by: tmonteil Owned by: jdemeyer
Priority: major Milestone: sage-8.1
Component: user interface Keywords:
Cc: vdelecroix Merged in:
Authors: Thierry Monteil, Jeroen Demeyer Reviewers: Sébastien Labbé
Report Upstream: N/A Work issues:
Branch: c88e146 (Commits) Commit:
Dependencies: #23985 Stopgaps:

Description (last modified by jdemeyer)

Since rst2ipynb version 0.2.2 supports a command line of the form rst2ipynb input output, there is no longer a need for the sage-rst2ipynb wrapper.

Change History (13)

comment:1 Changed 3 years ago by tmonteil

  • Branch set to u/tmonteil/let__sage__rst2ipynb__output_error_messages_provided_by_rst2ipynb

comment:2 Changed 3 years ago by tmonteil

  • Commit set to 62c625d0da928a9588ecfe489c7462a8ed7b11d1
  • Status changed from new to needs_review

There should be a way to first print the help and then print the error with some fancy redirections, but unless someone knows how to do it, let us move forward.


New commits:

62c625d#23415 : remove stderr redirection.

comment:3 Changed 3 years ago by tmonteil

  • Component changed from PLEASE CHANGE to user interface

comment:4 Changed 2 years ago by jdemeyer

Why are you even adding || ( echo -e '\n' ; help ) ? I would go for

exec rst2ipynb --kernel='sagemath' ${1};;

comment:5 Changed 2 years ago by jdemeyer

I think that upgrading rst2ipynb to version 0.2.2 (#23985) would remove the need for the sage-rst2ipynb script in the first place.

comment:6 Changed 2 years ago by jdemeyer

  • Dependencies set to #23985
  • Description modified (diff)
  • Status changed from needs_review to needs_work
  • Summary changed from Let 'sage -rst2ipynb' output error messages provided by rst2ipynb to Remove sage-rst2ipynb

comment:7 Changed 2 years ago by jdemeyer

  • Branch changed from u/tmonteil/let__sage__rst2ipynb__output_error_messages_provided_by_rst2ipynb to u/jdemeyer/let__sage__rst2ipynb__output_error_messages_provided_by_rst2ipynb

comment:8 Changed 2 years ago by jdemeyer

  • Authors changed from Thierry Monteil to Thierry Monteil, Jeroen Demeyer
  • Commit changed from 62c625d0da928a9588ecfe489c7462a8ed7b11d1 to c88e146a86625ab7b131e8d00f91307c94acde22
  • Status changed from needs_work to needs_review

New commits:

4a31dbfUpgrade rst2ipynb to version 0.2.2 and check pandoc dependency
c88e146Remove sage-rst2ipynb script

comment:9 follow-up: Changed 2 years ago by slabbe

I tested sage -rst2ipynb on a file I created and everything is fine. I get All tests passed with make ptestlong. I was going to give positive review, put I have one issue: after installing rst2ipynb, rst2ipynb is not part of my default list of optional packages to test:

slabbe@labbe-laptop sage $ sage -i rst2ipynb
make build/make/Makefile
make[1] : on entre dans le répertoire « /home/slabbe/GitBox/sage »
make[1]: « build/make/Makefile » est à jour.
make[1] : on quitte le répertoire « /home/slabbe/GitBox/sage »
build/bin/sage-logger \
	"cd build/make && ./install 'all-toolchain'" logs/install.log
Nothing to (re)build / all up-to-date.

make build/make/Makefile
make[1] : on entre dans le répertoire « /home/slabbe/GitBox/sage »
make[1]: « build/make/Makefile » est à jour.
make[1] : on quitte le répertoire « /home/slabbe/GitBox/sage »
build/bin/sage-logger \
	"cd build/make && ./install 'rst2ipynb'" logs/install.log
Nothing to (re)build / all up-to-date.
slabbe@labbe-laptop sage $ sage -t --show-skipped src/sage/tests/cmdline.py 
Running doctests with ID 2017-10-19-15-33-52-a53fc644.
Git branch: 23415
Using --optional=ccache,mpir,pandoc_attributes,pandocfilters,python2,sage
Doctesting 1 file.
sage -t --warn-long 53.9 src/sage/tests/cmdline.py
    3 gdb tests not run
    12 internet tests not run
    4 kash tests not run
    6 rst2ipynb tests not run
    [244 tests, 29.19 s]
----------------------------------------------------------------------
All tests passed!
----------------------------------------------------------------------
Total time for all tests: 29.2 seconds
    cpu time: 0.5 seconds
    cumulative wall time: 29.2 seconds

why?

comment:10 in reply to: ↑ 9 Changed 2 years ago by jdemeyer

  • Owner changed from (none) to jdemeyer
  • Reviewers set to Sébastien Labbé

Replying to slabbe:

after installing rst2ipynb, rst2ipynb is not part of my default list of optional packages to test:

Fixed in #23997

comment:11 Changed 2 years ago by slabbe

  • Status changed from needs_review to positive_review

comment:12 Changed 2 years ago by vbraun

  • Branch changed from u/jdemeyer/let__sage__rst2ipynb__output_error_messages_provided_by_rst2ipynb to c88e146a86625ab7b131e8d00f91307c94acde22
  • Resolution set to fixed
  • Status changed from positive_review to closed

comment:13 Changed 20 months ago by slabbe

  • Commit c88e146a86625ab7b131e8d00f91307c94acde22 deleted

follow-up ticket: #25537

Note: See TracTickets for help on using tickets.