Opened 3 years ago

Closed 3 years ago

#24286 closed defect (fixed)

py3: minor fixes to sage.repl.load and sage.repl.attach

Reported by: embray Owned by:
Priority: major Milestone: sage-8.2
Component: python3 Keywords:
Cc: jdemeyer Merged in:
Authors: Erik Bray Reviewers: Jeroen Demeyer
Report Upstream: N/A Work issues:
Branch: ede1402 (Commits, GitHub, GitLab) Commit: ede1402a8ec9f4a2012584b3017d54e1c925e692
Dependencies: Stopgaps:

Status badges

Description

This will be one of many tickets I will be opening for miscellaneous minor fixes to specific modules.

In some cases I'll keep these more "thematic" in which case they might span multiple modules, but in other cases it is simpler to submit one module at a time for minor fixes.

See the commit messages for more details about individual fixes.

Change History (21)

comment:1 Changed 3 years ago by embray

  • Dependencies set to #24222

This will need #24222 to be merged first to make sense.

comment:2 Changed 3 years ago by embray

  • Status changed from new to needs_review

comment:3 Changed 3 years ago by chapoton

  • Cc jdemeyer added

looks good enough to me.

Jeroen, any opinion ?

EDIT: This seems to break several things, see patchbot report..

Last edited 3 years ago by chapoton (previous) (diff)

comment:4 Changed 3 years ago by embray

  • Status changed from needs_review to needs_work

comment:5 Changed 3 years ago by chapoton

  • Branch changed from u/embray/python3/sage-repl-load to public/python3sage-repl-load
  • Commit changed from 5d3dce1a7e73ff7ac81a87d822f2651a6273e8f5 to 480468942951a7b55226e535e4ec8e05d045ee3d

I fixed one of the doctest failures (the easy one).


New commits:

4eb7374Merge branch 'u/embray/python3/sage-repl-load' in 8.2.b2
4804689fix one failing doctest

comment:6 Changed 3 years ago by embray

  • Branch changed from public/python3sage-repl-load to u/embray/python3/sage-repl-load
  • Commit changed from 480468942951a7b55226e535e4ec8e05d045ee3d to 5d3dce1a7e73ff7ac81a87d822f2651a6273e8f5

I already had this fixed in my local branch; I just hadn't pushed it yet, so for simplicity's sake I'll keep my branch. The other issues are (mostly) related to #24475

comment:7 Changed 3 years ago by embray

  • Summary changed from py3: minor fixes to sage.repl.load to py3: minor fixes to sage.repl.load and sage.repl.attach

comment:8 Changed 3 years ago by chapoton

  • Branch changed from u/embray/python3/sage-repl-load to public/python3/sage-repl-load
  • Commit changed from 5d3dce1a7e73ff7ac81a87d822f2651a6273e8f5 to 78e55f92887ca824dc802558490e14c40487607c
  • Status changed from needs_work to needs_review

New commits:

78e55f9Merge branch 'u/embray/python3/sage-repl-load' in 8.1.b5

comment:9 Changed 3 years ago by chapoton

  • Dependencies #24222 deleted

comment:10 Changed 3 years ago by git

  • Commit changed from 78e55f92887ca824dc802558490e14c40487607c to 236f2a82158e430da95af81bfe52b5dadc740d53

Branch pushed to git repo; I updated commit sha1. New commits:

236f2a8fix doctests

comment:11 Changed 3 years ago by jdemeyer

  • Status changed from needs_review to needs_work
  1. In src/sage/repl/load.py, some doctests are marked # py2 for no good reason.
  1. Why use tmp_filename here?
            sage: fname = tmp_filename(ext='.py')
            sage: fullpath = os.path.join(t_dir, fname)
            sage: with open(fullpath, 'w') as f:
            ....:     _ = f.write("print(37 * 3)")
    
  1. The git history is a mess (in particular, the commit message Merge branch 'u/embray/python3/sage-repl-load' in 8.1.b5 is wrong). It would be best to squash this in one commit on top op Sage 8.2.beta5

comment:12 Changed 3 years ago by git

  • Commit changed from 236f2a82158e430da95af81bfe52b5dadc740d53 to be956dfdae833dd193396c8d45c2c3869ff15277

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

be956df# Ceci est la combinaison de 2 commits.

comment:13 follow-up: Changed 3 years ago by chapoton

I do not understand the issue in point 2

comment:14 Changed 3 years ago by git

  • Commit changed from be956dfdae833dd193396c8d45c2c3869ff15277 to 040cc3ff694bfc05ee9a7dff0b14025642335204

Branch pushed to git repo; I updated commit sha1. New commits:

040cc3ftrac 24286 fixing doctest

comment:15 Changed 3 years ago by chapoton

green bot

comment:16 in reply to: ↑ 13 ; follow-up: Changed 3 years ago by jdemeyer

Replying to chapoton:

I do not understand the issue in point 2

The old code did not use tmp_filename. The new code does. I see no reason for that change, so it needs to be justified.

comment:17 Changed 3 years ago by git

  • Commit changed from 040cc3ff694bfc05ee9a7dff0b14025642335204 to ede1402a8ec9f4a2012584b3017d54e1c925e692

Branch pushed to git repo; I updated commit sha1. New commits:

ede1402trac 24286 detail

comment:18 Changed 3 years ago by chapoton

  • Status changed from needs_work to needs_review

ok, done

comment:19 Changed 3 years ago by jdemeyer

  • Reviewers set to Jeroen Demeyer
  • Status changed from needs_review to positive_review

comment:20 in reply to: ↑ 16 Changed 3 years ago by embray

Replying to jdemeyer:

Replying to chapoton:

I do not understand the issue in point 2

The old code did not use tmp_filename. The new code does. I see no reason for that change, so it needs to be justified.

I think it was probably a mistake but I'm going to have to double-check now...

comment:21 Changed 3 years ago by vbraun

  • Branch changed from public/python3/sage-repl-load to ede1402a8ec9f4a2012584b3017d54e1c925e692
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.