Opened 2 years ago
Closed 2 years 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, GitHub, GitLab) | Commit: | d73a9fed2ca4e5d4f55115de8fbbb538e862034a |
Dependencies: | Stopgaps: |
Description (last modified by )
Change History (28)
comment:1 Changed 2 years ago by
- Branch set to u/chapoton/26213
- Commit set to 172458e6e98f382ea49d872ee09ecf635caff9ef
- Status changed from new to needs_review
comment:2 Changed 2 years ago by
The two tests use the doctest tag # py2
. Therefore it fails with python2.
comment:3 Changed 2 years ago by
- Reviewers set to Vincent Klein
comment:4 Changed 2 years ago by
- Commit changed from 172458e6e98f382ea49d872ee09ecf635caff9ef to a6918b22cb1ec71df9724f271e628df49099a6fb
Branch pushed to git repo; I updated commit sha1. New commits:
a6918b2 | fix
|
comment:5 Changed 2 years ago by
thanks, fixed
comment:6 Changed 2 years ago by
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 2 years ago by
- Cc embray jdemeyer added
Then let us ask for help.. Erik or Jeroen, any idea ?
comment:8 Changed 2 years ago by
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 2 years ago by
- Commit changed from a6918b22cb1ec71df9724f271e628df49099a6fb to e462f1a47cff55b24e100a2ffd01f958623368fb
Branch pushed to git repo; I updated commit sha1. New commits:
e462f1a | fix for py3
|
comment:10 Changed 2 years ago by
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 2 years ago by
Indeed, it's the #25676 ticket.
comment:12 Changed 2 years ago by
- Description modified (diff)
comment:13 Changed 2 years ago by
- Description modified (diff)
It's ok for me. I let Eric pass it in positive review.
comment:14 Changed 2 years ago by
comment:15 Changed 2 years ago by
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 2 years ago by
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 2 years ago by
Weird. I don't even have that test locally. Was it just added in 8.4.beta4?
comment:18 Changed 2 years ago by
last modified seven years ago..
comment:19 Changed 2 years ago by
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 2 years ago by
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: 124 124 True 125 125 """ 126 126 return not (self == other) 127 127 128 128 129 129 class Stock: 130 130 """ … … class Stock: 562 562 1212407640 187.75 188.00 187.75 188.00 2000, 563 563 1212405780 187.80 187.80 187.80 187.80 100 564 564 ] 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'572 565 """ 573 file_obj = open(file, 'r') 574 R = file_obj.read(); 566 with open(file, 'r') as fobj: 567 R = fobj.read() 568 575 569 self.__historical = self._load_from_csv(R) 576 file_obj.close()577 570 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 2 years ago by
For that matter the 'r'
argument to open()
is superfluous as well.
comment:22 Changed 2 years ago by
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 2 years ago by
ok. Let us remove the useless doctest here.
comment:24 Changed 2 years ago by
- Commit changed from e462f1a47cff55b24e100a2ffd01f958623368fb to d73a9fed2ca4e5d4f55115de8fbbb538e862034a
Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
d73a9fe | py3: fix finance doctest
|
comment:25 Changed 2 years ago by
done, ready for review again
comment:26 Changed 2 years ago by
- 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:28 Changed 2 years ago by
- Branch changed from u/chapoton/26213 to d73a9fed2ca4e5d4f55115de8fbbb538e862034a
- Resolution set to fixed
- Status changed from positive_review to closed
New commits:
py3: fix doctests in finance