Opened 4 years ago

Closed 4 years ago

#24046 closed enhancement (fixed)

py3 : fixing exit

Reported by: chapoton Owned by:
Priority: major Milestone: sage-8.1
Component: python3 Keywords:
Cc: jdemeyer, tscrim, jhpalmieri, embray Merged in:
Authors: Frédéric Chapoton Reviewers: Erik Bray
Report Upstream: N/A Work issues:
Branch: 8d15a11 (Commits, GitHub, GitLab) Commit: 8d15a11b50a371d9fcb7f31414f02fb1e001c97c
Dependencies: Stopgaps:

Status badges

Description

now that sage starts with python3

Change History (25)

comment:1 Changed 4 years ago by chapoton

  • Branch set to u/chapoton/24046
  • Cc jdemeyer tscrim jhpalmieri embray added
  • Commit set to 931a3313a382607e503c5fc85a1effb0f956481f
  • Status changed from new to needs_review

New commits:

931a331fixing exit with python3

comment:2 Changed 4 years ago by chapoton

green bot, please review

comment:3 Changed 4 years ago by jdemeyer

Can you tell what goes wrong without this patch? I don't like to fix bugs that I do not understand.

comment:4 Changed 4 years ago by chapoton

I will not have access to a python3-build of sage until next week-end.

As far as I remember, when typing "exit", it complains that it was expecting bytes or str and not a lazy string.

You can try and see with the experimental branch (see the recent sage-devel thread)

comment:5 Changed 4 years ago by embray

This seems fine for now, but it got me thinking--instead of using str(lazy_str) everywhere (not that there are many examples of this, but I'm sure there are a few) we could kill all birds at once by defining __fspath__ on lazy_string. That way, lazy_strings can be used transparently with os.path and any other interfaces that support the PathLike ABC (which most path-related interfaces on Python 3 now do, at least in the stdlib).

comment:6 follow-up: Changed 4 years ago by jhpalmieri

Is this just a matter of adding __fspath__ = __str__ in the _LazyString class?

comment:7 Changed 4 years ago by chapoton

so, is this a positive review ?

comment:8 Changed 4 years ago by chapoton

bot is green..

comment:9 in reply to: ↑ 6 Changed 4 years ago by embray

Replying to jhpalmieri:

Is this just a matter of adding __fspath__ = __str__ in the _LazyString class?

I think so. I suggest we try this instead of the explicit fix in this ticket.

comment:10 Changed 4 years ago by jhpalmieri

  • Branch changed from u/chapoton/24046 to u/jhpalmieri/24046

comment:11 Changed 4 years ago by git

  • Commit changed from 931a3313a382607e503c5fc85a1effb0f956481f to a7fc4de889cb01787667d2802b013be7fe801894

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

a7fc4detrac 24046: for lazy strings, define __fspath__ to be an alias for __str__

comment:12 Changed 4 years ago by jhpalmieri

Please try this version with Python 3 instead. If it doesn't work, then positive review for your solution.

comment:13 Changed 4 years ago by jdemeyer

I would much prefer

def __fspath__(self):
    """
    Docstring here
    """
    return str(self)

This works better with inheritance and it allows adding a docstring.

comment:14 Changed 4 years ago by git

  • Commit changed from a7fc4de889cb01787667d2802b013be7fe801894 to 19bf1531a14baa9532aa88da8c77fb2e1866b98c

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

19bf153trac 24046: define __fspath__ for lazy strings.

comment:15 Changed 4 years ago by jhpalmieri

How about this?

comment:16 Changed 4 years ago by jdemeyer

  • Status changed from needs_review to needs_info

Isn't the whole point of this ticket that os.path.join(SAGE_TMP, 'hello') should work without adding str()?

comment:17 Changed 4 years ago by jhpalmieri

Sorry, I think the doctest should not include str. My mistake.

Last edited 4 years ago by jhpalmieri (previous) (diff)

comment:18 Changed 4 years ago by jhpalmieri

  • Status changed from needs_info to needs_review

comment:19 Changed 4 years ago by git

  • Commit changed from 19bf1531a14baa9532aa88da8c77fb2e1866b98c to 5be9b4f3e092d1d7534cf9ea7bbab2c0cfd28e93

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

5be9b4ftrac 24046: fix doctest

comment:20 Changed 4 years ago by chapoton

There is a syntax error in doctests

comment:21 Changed 4 years ago by git

  • Commit changed from 5be9b4f3e092d1d7534cf9ea7bbab2c0cfd28e93 to 8d15a11b50a371d9fcb7f31414f02fb1e001c97c

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

8d15a11trac 24046: fix doctest again

comment:22 Changed 4 years ago by jhpalmieri

You're right, I keep making mistakes here.

comment:23 Changed 4 years ago by embray

  • Reviewers set to Erik Bray

Looks good to me, but I'll let Frédéric sign off on it since he's in a better position to test it.

comment:24 Changed 4 years ago by chapoton

  • Status changed from needs_review to positive_review

comment:25 Changed 4 years ago by vbraun

  • Branch changed from u/jhpalmieri/24046 to 8d15a11b50a371d9fcb7f31414f02fb1e001c97c
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.