Opened 8 months ago

Closed 7 months ago

#26213 closed enhancement (fixed)

py3: minor cleanup in sage.finance.stock

Reported by: chapoton Owned by:
Priority: trivial Milestone: sage-8.4
Component: python3 Keywords:
Cc: embray, jdemeyer Merged in:
Authors: Frédéric Chapoton Reviewers: Vincent Klein
Report Upstream: N/A Work issues:
Branch: d73a9fe (Commits) Commit: d73a9fed2ca4e5d4f55115de8fbbb538e862034a
Dependencies: Stopgaps:

Change History (28)

comment:1 Changed 8 months ago by chapoton

  • Branch set to u/chapoton/26213
  • Commit set to 172458e6e98f382ea49d872ee09ecf635caff9ef
  • Status changed from new to needs_review

New commits:

172458epy3: fix doctests in finance

comment:2 Changed 8 months ago by vklein

The two tests use the doctest tag # py2. Therefore it fails with python2.

Last edited 8 months ago by vklein (previous) (diff)

comment:3 Changed 8 months ago by vklein

  • Reviewers set to Vincent Klein

comment:4 Changed 8 months ago by git

  • Commit changed from 172458e6e98f382ea49d872ee09ecf635caff9ef to a6918b22cb1ec71df9724f271e628df49099a6fb

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

a6918b2fix

comment:5 Changed 8 months ago by chapoton

thanks, fixed

comment:6 Changed 8 months ago by vklein

With python3, i get :

sage -t src/sage/finance/stock.py
**********************************************************************
File "src/sage/finance/stock.py", line 573, in sage.finance.stock.Stock.load_from_file
Failed example:
    finance.Stock("AAPL").load_from_file("I am not a file") # py3
Expected:
    Traceback (most recent call last):
    ...
    FileNotFoundError: [Errno 2] No such file or directory: 'I am not a file'
Got:
    <BLANKLINE>
    Traceback (most recent call last):
      File "/home/vklein/odk/sage/local/lib/python3.6/site-packages/sage/doctest/forker.py", line 650, in _run
        self.compile_and_execute(example, compiler, test.globs)
      File "/home/vklein/odk/sage/local/lib/python3.6/site-packages/sage/doctest/forker.py", line 1061, in compile_and_execute
        exec(compiled, globs)
      File "<doctest sage.finance.stock.Stock.load_from_file[4]>", line 1, in <module>
        finance.Stock("AAPL").load_from_file("I am not a file") # py3
      File "/home/vklein/odk/sage/local/lib/python3.6/site-packages/sage/finance/stock.py", line 578, in load_from_file
        file_obj = open(file, 'r')
    FileNotFoundError: [Errno 2] No such file or directory: 'I am not a file'

And if i add a <BLANKLINE> in the doctest expected result :

sage -t src/sage/finance/stock.py
**********************************************************************
File "src/sage/finance/stock.py", line 573, in sage.finance.stock.Stock.load_from_file
Failed example:
    finance.Stock("AAPL").load_from_file("I am not a file") # py3
Exception raised:
    Traceback (most recent call last):
      File "/home/vklein/odk/sage/local/lib/python3.6/site-packages/sage/doctest/forker.py", line 650, in _run
        self.compile_and_execute(example, compiler, test.globs)
      File "/home/vklein/odk/sage/local/lib/python3.6/site-packages/sage/doctest/forker.py", line 1061, in compile_and_execute
        exec(compiled, globs)
      File "<doctest sage.finance.stock.Stock.load_from_file[4]>", line 1, in <module>
        finance.Stock("AAPL").load_from_file("I am not a file") # py3
      File "/home/vklein/odk/sage/local/lib/python3.6/site-packages/sage/finance/stock.py", line 579, in load_from_file
        file_obj = open(file, 'r')
    FileNotFoundError: [Errno 2] No such file or directory: 'I am not a file'

It behave as if the Error is not catched by the doctest framework.

comment:7 Changed 8 months ago by chapoton

  • Cc embray jdemeyer added

Then let us ask for help.. Erik or Jeroen, any idea ?

comment:8 Changed 8 months ago by vklein

To simplify i have tested the following doctest :

sage: raise NameError("test")
Traceback (most recent call last):
...
NameError: test
sage: raise FileNotFoundError("test")
Traceback (most recent call last):
...
FileNotFoundError: test

The first test pass and the second fails.

comment:9 Changed 7 months ago by git

  • Commit changed from a6918b22cb1ec71df9724f271e628df49099a6fb to e462f1a47cff55b24e100a2ffd01f958623368fb

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

e462f1afix for py3

comment:10 Changed 7 months ago by chapoton

ok. So the reason is that there is in sage doctesting framework a mechanism that modifies some subexceptions of OSError in python3, and replace them with OSError.

Should be good to go now. Unless Erik (@embray) has a better idea ?

comment:11 Changed 7 months ago by vklein

Indeed, it's the #25676 ticket.

comment:12 Changed 7 months ago by vklein

  • Description modified (diff)

comment:13 Changed 7 months ago by vklein

  • Description modified (diff)

It's ok for me. I let Eric pass it in positive review.

comment:14 Changed 7 months ago by embray

Because of #25676 this shouldn't require separate #py2 and #py3 cases at all, unless I missed a case in #25676.

comment:15 Changed 7 months ago by chapoton

Well, I tried and it did not work. Because you are busy on other stuff, I thought it was good enough for the moment.

comment:16 Changed 7 months ago by embray

This is pretty low priority though (I would argue even trivial) so I don't see any reason to rush to a fix. I can have a look at it.

comment:17 Changed 7 months ago by embray

Weird. I don't even have that test locally. Was it just added in 8.4.beta4?

comment:18 Changed 7 months ago by chapoton

last modified seven years ago..

comment:19 Changed 7 months ago by embray

Oh I see now. In my branch I just removed that test completely because it was pointless. All it's testing is that the open(...) built-in raises an exception if the given file does not exist (duh). There's nothing in that code that even does anything special in that case, so the test is not useful at all.

comment:20 Changed 7 months ago by embray

I have some other minor cleanup in that file as well, such as replacing open() with with open(). I think I missed the spurious semicolon you had. This is my diff to the file:

  • src/sage/finance/stock.py

    diff --git a/src/sage/finance/stock.py b/src/sage/finance/stock.py
    index 1c9ba47..dfab025 100644
    a b class OHLC: 
    124124            True
    125125        """
    126126        return not (self == other)
    127 
     127
    128128
    129129class Stock:
    130130    """
    class Stock: 
    562562            1212407640 187.75 188.00 187.75 188.00       2000,
    563563            1212405780 187.80 187.80 187.80 187.80        100
    564564            ]
    565 
    566         This tests a file that doesn't exist::
    567 
    568             sage: finance.Stock("AAPL").load_from_file("I am not a file")
    569             Traceback (most recent call last):
    570             ...
    571             IOError: [Errno 2] No such file or directory: 'I am not a file'
    572565        """
    573         file_obj = open(file, 'r')
    574         R = file_obj.read();
     566        with open(file, 'r') as fobj:
     567            R = fobj.read()
     568
    575569        self.__historical = self._load_from_csv(R)
    576         file_obj.close()
    577570        return self.__historical

Still weird that it's failing for you though. I might temporarily restore the test just to investigate a little. But I still think it should just be deleted entirely.

comment:21 Changed 7 months ago by embray

For that matter the 'r' argument to open() is superfluous as well.

comment:22 Changed 7 months ago by embray

Ah, I see the problem. There are two classes of exception name mappings surrounding OSError: One is classes in Python 2 that have just been replaced with OSError (of which IOError happens to be one). The other is new-to-Python 3 specialized subclasses of OSError for specific errnos, such as FileNotFoundError.

On Python 3 the doctester code treats either one case, or the other. But in this case it should treat both by mapping IOError -> OSError -> FileNotFoundError. I'll make a ticket.

comment:23 Changed 7 months ago by chapoton

ok. Let us remove the useless doctest here.

comment:24 Changed 7 months ago by git

  • Commit changed from e462f1a47cff55b24e100a2ffd01f958623368fb to d73a9fed2ca4e5d4f55115de8fbbb538e862034a

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

d73a9fepy3: fix finance doctest

comment:25 Changed 7 months ago by chapoton

done, ready for review again

comment:26 Changed 7 months ago by embray

  • Status changed from needs_review to positive_review
  • Summary changed from py3: fix doctests in finance/ to py3: minor cleanup in sage.finance.stock

comment:27 Changed 7 months ago by embray

  • Priority changed from major to trivial

In case it helps Volker.

comment:28 Changed 7 months ago by vbraun

  • Branch changed from u/chapoton/26213 to d73a9fed2ca4e5d4f55115de8fbbb538e862034a
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.