sage.features.Executable: Add method absolute_filename
mkoeppe opened this issue · 53 comments
We refactor Executable and StaticFile through a base class FileFeature.
The method absolute_filename replaces StaticFile.absolute_path and is now also available for Executable. (This is for #33440/#31296/#30818.)
The renaming from StaticFile.absolute_path to absolute_filename (with deprecation) is preparation for reclaiming the name absolute_path for a method that returns a Path instead of a str.
We also add some type annotations.
Follow-up: #31296
CC: @tobiasdiez @fchapoton @seblabbe @orlitzky @kiwifb @antonio-rojas @tornaria
Component: refactoring
Author: Matthias Koeppe
Branch/Commit: e9568e9
Reviewer: Tobias Diez
Issue created by migration from https://trac.sagemath.org/ticket/31292
Setting new milestone based on a cursory review of ticket status, priority, and last modification date.
Description changed:
---
+++
@@ -1 +1,4 @@
-In particular, we change `absolute_path` to return a `Path`
+In particular, we change `StaticFile.absolute_path` to return a `Path`.
+
+We also add a method `Executable.absolute_path`. (This is for #33440/#31296/#30818.)
+Author: Matthias Koeppe
Replying to @mkoeppe:
In particular, we change
StaticFile.absolute_pathto return aPath.
Actually this would lead to errors like
File "src/sage/databases/cremona.py", line 1552, in sage.databases.cremona.LargeCremonaDatabase.degphi
Failed example:
c = CremonaDatabase()
Exception raised:
...
File "/Users/mkoeppe/s/sage/sage-rebasing/worktree-gcc11/local/var/lib/sage/venv-python3.9/lib/python3.9/site-packages/sage/databases/sql_db.py", line 1066, in __init__
elif (filename[-3:] != '.db'):
TypeError: 'PosixPath' object is not subscriptable
Description changed:
---
+++
@@ -1,4 +1,4 @@
-In particular, we change `StaticFile.absolute_path` to return a `Path`.
+We add a method `Executable.absolute_path`, in analogy to `StaticFile`. (This is for #33440/#31296/#30818.)
-We also add a method `Executable.absolute_path`. (This is for #33440/#31296/#30818.)
+We also add some type annotations.
Branch pushed to git repo; I updated commit sha1. New commits:
88860d3 | sage.features.Executable.absolute_path: If SAGE_LOCAL != SAGE_VENV, search first in SAGE_VENV/bin, SAGE_LOCAL/bin, then PATH |
Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
Description changed:
---
+++
@@ -2,3 +2,5 @@
We also add some type annotations.
+Follow-up: #31296
+Branch pushed to git repo; I updated commit sha1. New commits:
8bb7e3a | src/sage/features/__init__.py: Relax doctest |
Replying to @mkoeppe:
Replying to @mkoeppe:
In particular, we change
StaticFile.absolute_pathto return aPath.Actually this would lead to errors like
File "src/sage/databases/cremona.py", line 1552, in sage.databases.cremona.LargeCremonaDatabase.degphi Failed example: c = CremonaDatabase() Exception raised: ... File "/Users/mkoeppe/s/sage/sage-rebasing/worktree-gcc11/local/var/lib/sage/venv-python3.9/lib/python3.9/site-packages/sage/databases/sql_db.py", line 1066, in __init__ elif (filename[-3:] != '.db'): TypeError: 'PosixPath' object is not subscriptable
This is to be expected, since it's a path and not a string that you work with. The extension can easily be obtained using the suffix method: https://docs.python.org/3/library/pathlib.html#pathlib.PurePath.suffixes
Replying to @mkoeppe:
Summary changed from sage.features.StaticFile, Executable: Use pathlib, add type annotations to sage.features.Executable: Add method absolute_path
This completely changes the scope of this ticket whose original intention was to migrate sage.features.absolute_path to return Path.
def absolute_path(self) -> str:
why not make this return a pathlib.Path?
reason="Executable {executable!r} not found on PATH.".format(executable=self.executable),
use f-strings.
Replying to @tobiasdiez:
def absolute_path(self) -> str:why not make this return a
pathlib.Path?
Because that breaks the public API.
Replying to @tobiasdiez:
Replying to @mkoeppe:
Summary changed from sage.features.StaticFile, Executable: Use pathlib, add type annotations to sage.features.Executable: Add method absolute_path
This completely changes the scope of this ticket whose original intention was to migrate sage.features.absolute_path to return
Path.
Yes, when I wrote this ticket I thought that could be done compatibly, but it can't
Replying to @mkoeppe:
Replying to @tobiasdiez:
Replying to @mkoeppe:
Summary changed from sage.features.StaticFile, Executable: Use pathlib, add type annotations to sage.features.Executable: Add method absolute_path
This completely changes the scope of this ticket whose original intention was to migrate sage.features.absolute_path to return
Path.Yes, when I wrote this ticket I thought that could be done compatibly, but it can't
Then just introduce a new method, and deprecate the old one.
Replying to @mkoeppe:
Replying to @tobiasdiez:
use f-strings.
.formatis not deprecated or anything.
That's right, but f-strings are still easier to read.
Replying to @tobiasdiez:
Then just introduce a new method, and deprecate the old one.
I was in the middle of doing this but couldn't come up with a name for the new method, given that the good name, absolute_path, is already taken. Suggestions?
Replying to @tobiasdiez:
That's right, but f-strings are still easier to read.
I like f-strings too, but in the interest of focus I don't change everything that can be changed. This line of code is copied from a place in the same file.
Description changed:
---
+++
@@ -1,4 +1,8 @@
-We add a method `Executable.absolute_path`, in analogy to `StaticFile`. (This is for #33440/#31296/#30818.)
+We refactor `Executable` and `StaticFile` through a base class `FileFeature`.
+
+The method `absolute_filename` replaces `StaticFile.absolute_path` and is now also available for `Executable`. (This is for #33440/#31296/#30818.)
+
+The renaming from `StaticFile.absolute_path` to `absolute_filename` (with deprecation) is preparation for reclaiming the name `absolute_path` for a method that returns a `Path` instead of a `str`.
We also add some type annotations.
EXAMPLES::
sage: from sage.features import StaticFile, Executable, FileFeature
sage: issubclass(StaticFile, FileFeature)
True
sage: issubclass(Executable, FileFeature)
True
Please replace this by a meaningful doctest that actually shows how to use the class.
In FileFeature.absolute_path you call self.absolute_filename which doesn't exist. So this should be added at least as an abstract method.
try:
from sage.misc.superseded import deprecation
except ImportError:
pass
deprecation(31292, 'method absolute_path has been replaced by absolute_filename')
That looks like a strange construction. Why should the import of the deprecation fail? But even if it fails the call of deprecation will throw another error as its not defined then.
Branch pushed to git repo; I updated commit sha1. New commits:
66d79f8 | sage.features.FileFeature.absolute_path: Fixup try...except...else |
Replying to @tobiasdiez:
try: from sage.misc.superseded import deprecation except ImportError: pass deprecation(31292, 'method absolute_path has been replaced by absolute_filename')That looks like a strange construction. Why should the import of the deprecation fail?
Because #29941 puts sage.features into a distribution sagemath-environment, which does not install-requires sagemath-objects, which provides sage.misc.superseded.
But even if it fails the call of
deprecationwill throw another error as its not defined then.
Thanks, forgot the else:, fixed now.
Replying to @tobiasdiez:
In FileFeature.absolute_path you call
self.absolute_filenamewhich doesn't exist. So this should be added at least as an abstract method.
Avoiding use of sage.misc.abstract_method for the same modularization reason.
I'll not start using https://docs.python.org/3/library/abc.html here for this purpose.
I can make an ad-hoc abstract method that just raises a NotImplementedError
Branch pushed to git repo; I updated commit sha1. New commits:
cad9f0f | sage.features.FileFeature.absolute_filename: New abstract method |
Branch pushed to git repo; I updated commit sha1. New commits:
57531d0 | src/sage/features/__init__.py: Document why importing sage.misc.superseded can fail |
Branch pushed to git repo; I updated commit sha1. New commits:
66b8125 | sage.features.FileFeature: Add doc |
Replying to @tobiasdiez:
Please replace this by a meaningful doctest that actually shows how to use the class.
Done now
Branch pushed to git repo; I updated commit sha1. New commits:
d631a03 | src/sage/features/__init__.py: Fix docstring markup |
Thanks for the follow-up!
One last comment, otherwise it looks good to me:
The docs for absolute_filename in FileFeature now still refers to "executable", which should be replaced by "feature".
By the way, do the inheriting classes (Executable and StaticFile) really need to provide docstrings? Sphinx should copy them from the base class or not?
Branch pushed to git repo; I updated commit sha1. New commits:
e9568e9 | FileFeature.absolute_filename: Fix docstring |
Replying to @tobiasdiez:
By the way, do the inheriting classes (Executable and StaticFile) really need to provide docstrings?
Yes, per our style guide.
Replying to @mkoeppe:
Replying to @tobiasdiez:
By the way, do the inheriting classes (Executable and StaticFile) really need to provide docstrings?
Yes, per our style guide.
What is the reason behind this requirement/convention?
Reviewer: Tobias Diez
Thanks for reviewing!
Replying to @tobiasdiez:
What is the reason behind this requirement/convention?
It was already there when I started Sage dev. It's a bit annoying at times and leads to a lot of copy+paste, but it does force people to write doctests. And having the policy scales better than renegotiating the style with every new developer.
Changed branch from u/mkoeppe/sage_features_staticfile__executable__use_pathlib__add_type_annotations to e9568e9