#32164 closed enhancement (fixed)

more typing annotations in combinat folder

Reported by: Frédéric Chapoton Owned by:
Priority: major Milestone: sage-9.4
Component: combinatorics Keywords:
Cc: Tobias Diez, Travis Scrimshaw Merged in:
Authors: Frédéric Chapoton Reviewers: Matthias Koeppe
Report Upstream: N/A Work issues:
Branch: 79b2053 (Commits, GitHub, GitLab) Commit: 79b205373300669adfa8b9e891d1080391878108
Dependencies: Stopgaps:

Status badges

Description

Add minimal typing annotations in the files

src/sage/combinat/dyck_word.py
src/sage/combinat/e_one_star.py
src/sage/combinat/non_decreasing_parking_function.py
src/sage/combinat/tamari_lattices.py

Change History (9)

comment:1 Changed 17 months ago by Frédéric Chapoton

Branch: u/chapoton/32164
Commit: 8907bef91be430a0763db3a2648656351d50c315
Status: newneeds_review

New commits:

8907befadding typing annotations in some combinat/ files

comment:2 Changed 17 months ago by Matthias Köppe

Cc: Tobias Diez added
             heights[i + 1] = height
         return tuple(heights)
 
-    def associated_parenthesis(self, pos):
+    def associated_parenthesis(self, pos) -> Union[int, None]:
         r"""
         Report the position for the parenthesis in ``self`` that matches the
         one at position ``pos``.

I'm really only guessing here, but I think Optional[int] may be more idiomatic

comment:3 Changed 17 months ago by Frédéric Chapoton

It seems that the best syntax is int | None starting from python 3.10 :

https://stackoverflow.com/questions/51710037/how-should-i-use-the-optional-type-hint

comment:4 Changed 17 months ago by Matthias Köppe

Cc: Travis Scrimshaw added

Thanks for the pointer; it will be a while though until we can use features introduced in 3.10.

I'm not sure if I would agree with the API change here. This is a getter/setter-style method; I don't know if making a change just to be able to add a type decoration is justified:

-    def color(self, color=None):
+    def color(self, color=None) -> Color:
         r"""
         Return or change the color of the face.
 
         INPUT:
 
         - ``color`` - string, rgb tuple, color (optional, default: ``None``)
           the new color to assign to the face. If ``None``, it returns the
           color of the face.
 
         OUTPUT:
 
-            color
+        color
 
         EXAMPLES::
 
             sage: from sage.combinat.e_one_star import Face
             sage: f = Face((0,2,0), 3)
             sage: f.color()
             RGB color (0.0, 0.0, 1.0)
             sage: f.color('red')
-            sage: f.color()
             RGB color (1.0, 0.0, 0.0)
-
         """
-        if color is None:
-            return self._color
-        else:
+        if color is not None:
             self._color = Color(color)
+        return self._color

comment:5 Changed 17 months ago by Tobias Diez

The or syntax int | None should be possible already in 3.7 with the future import.

For the color function, if the change to always return something is not intended, this kind of situation is usually resolved by specifying overloads (https://docs.python.org/3/library/typing.html#typing.overload), i.e something like

@overload
def color(self, color: None) -> Color:
    ...
@overload
def color(self, color: Color) -> None:
    ...
def color(self, color):
   implementation

comment:6 Changed 17 months ago by git

Commit: 8907bef91be430a0763db3a2648656351d50c31579b205373300669adfa8b9e891d1080391878108

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

79b2053annotations details in e_one_star

comment:7 Changed 17 months ago by Frédéric Chapoton

I have undone the change to color and removed its annotation.

I also used the new syntax for union annotation elsewhere in the same file.

comment:8 Changed 17 months ago by Matthias Köppe

Reviewers: Matthias Koeppe
Status: needs_reviewpositive_review

LGTM and also passes tests also on Python 3.7.11.

comment:9 Changed 17 months ago by Volker Braun

Branch: u/chapoton/3216479b205373300669adfa8b9e891d1080391878108
Resolution: fixed
Status: positive_reviewclosed
Note: See TracTickets for help on using tickets.