Opened 3 years ago

Closed 3 years ago

Last modified 2 years ago

#27719 closed defect (fixed)

Preparser for file.sage: treat "from __future__ import ..." properly

Reported by: jhpalmieri Owned by:
Priority: minor Milestone: sage-8.8
Component: misc Keywords:
Cc: Merged in:
Authors: John Palmieri, Nils Bruin Reviewers: Nils Bruin, John Palmieri
Report Upstream: N/A Work issues:
Branch: 5d014bc (Commits, GitHub, GitLab) Commit:
Dependencies: Stopgaps:

Status badges

Description

If a file file.sage has a line from __future__ import ..., the preparser should put it at the top of the file where it needs to be.

Change History (12)

comment:1 Changed 3 years ago by jhpalmieri

  • Branch set to u/jhpalmieri/future

comment:2 Changed 3 years ago by jhpalmieri

  • Authors set to John Palmieri
  • Commit set to 4b627ff0c0a01359ced204a5d9c38680d9abe5b5
  • Status changed from new to needs_review

New commits:

4b627fftrac 27719: preparser handling of "from __future__ import ..."

comment:3 Changed 3 years ago by git

  • Commit changed from 4b627ff0c0a01359ced204a5d9c38680d9abe5b5 to 74c2e3babf01e516a4fd19ba8702e03e06a8dca6

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

74c2e3btrac 27719: preparser handling of "from __future__ import ..."

comment:4 Changed 3 years ago by git

  • Commit changed from 74c2e3babf01e516a4fd19ba8702e03e06a8dca6 to 8d8e713d2787037c2ef6bb131adfcee8e4f39ad4

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

8d8e713trac 27719: preparser handling of "from __future__ import ..."

comment:5 Changed 3 years ago by nbruin

  • I think in principle a "from future import" directive CAN use parentheses and thus cover multiple lines (I guess they could escape newlines as well to cover multiple lines). I think that's sufficiently rare that we can afford to miss the case, but perhaps we should document that we don't deal with it.
  • It would be quite a bit more efficient if we use a regexp that we can apply on the string as a whole, so that we can avoid splitting the file into lines and concatenating it later. We can then split the string on the index where the statement was found and do surgery that way. Then we only incur string manipulation overhead when we encounter a __future__.
  • If we're going to stick with splitting, then do the reordering manipulations on the list of lines and later assemble the string via a "\n".join(list of lines). That's O(n) rather than the O(n2) you get from repeatedly adding strings.

comment:6 Changed 3 years ago by git

  • Commit changed from 8d8e713d2787037c2ef6bb131adfcee8e4f39ad4 to d81351873c2e2132379389dbdaba91f07d0a972a

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

d813518trac 27719: preparser handling of "from __future__ import ..."

comment:7 Changed 3 years ago by jhpalmieri

Here is a different version which searches the whole string instead of splitting, and it does some crude error-checking looking for multi-line from __future__ import ... statements. If it finds them, it raises an error. I don't know how to do anything more sophisticated without actually parsing the Python code in the file, and that seems like overkill.

comment:8 Changed 3 years ago by nbruin

  • Branch changed from u/jhpalmieri/future to u/nbruin/future

comment:9 Changed 3 years ago by nbruin

  • Commit changed from d81351873c2e2132379389dbdaba91f07d0a972a to 5d014bc3aa72d7e2b3a95254fa23ca9a0318cbee

Made a tiny update: appending a few items to a list should be quite cheap (extra room should be available). The pythonic way of joining strings together is via "join" (it's faster; not that it matters here)

Let's see what the patchbots say. The previous reports had some failures (perhaps spurious). Otherwise: I'm OK with your changes. If you're OK with mine you can set it to positive on my behalf.


New commits:

5d014bcUse a slightly more pythonic way of joining multiple strings together

comment:10 Changed 3 years ago by jhpalmieri

  • Authors changed from John Palmieri to John Palmieri, Nils Bruin
  • Reviewers set to Nils Bruin, John Palmieri
  • Status changed from needs_review to positive_review

I see the same patchbot doctest errors here as on several other tickets, so they look spurious.

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

comment:11 Changed 3 years ago by vbraun

  • Branch changed from u/nbruin/future to 5d014bc3aa72d7e2b3a95254fa23ca9a0318cbee
  • Resolution set to fixed
  • Status changed from positive_review to closed

comment:12 Changed 2 years ago by embray

  • Commit 5d014bc3aa72d7e2b3a95254fa23ca9a0318cbee deleted

Whew, this looks fine, but we really need to replace this code with a real parser... :(

Note: See TracTickets for help on using tickets.