Opened 3 years ago

Closed 3 years ago

#27989 closed defect (fixed)

remove _r_action_ and _l_action_ from docs, and code

Reported by: Dima Pasechnik Owned by:
Priority: major Milestone: sage-8.9
Component: coercion Keywords:
Cc: Nicolas M. Thiéry, Simon King Merged in:
Authors: Frédéric Chapoton Reviewers: Dima Pasechnik
Report Upstream: N/A Work issues:
Branch: c1c09b8 (Commits, GitHub, GitLab) Commit: c1c09b8bcb84254c26743ca69579346a89cdffbe
Dependencies: Stopgaps:

Status badges

Description

_r_action_ and _l_action_ are in the docs, despite removed in #5597

quick grepping through src/sage/ shows that _r_action_ is never defined/used (only mentioned in commented out code or in messages), and _l_action_ is defined exactly once, in src/sage/rings/multi_power_series_ring_element.py and only used there, once.

So none of this is understood by any coersion/etc frameworks.

A documentation bug, I suppose (and probably a bug in src/sage/rings/multi_power_series_ring_element.py, which uses wrongly named stuff...)

Change History (16)

comment:1 Changed 3 years ago by Dima Pasechnik

Cc: Nicolas M. Thiéry Simon King added

comment:2 Changed 3 years ago by Simon King

I would somehow prefer to make the whole "action" business flexible enough that the parent of a method can define via categories (I think that's the point that currently is lacking) which method of the element shall be used for the action. But I definitely should dive into the code before giving any recommendations...

comment:3 Changed 3 years ago by Erik Bray

Milestone: sage-8.8

As the Sage-8.8 release milestone is pending, we should delete the sage-8.8 milestone for tickets that are not actively being worked on or that still require significant work to move forward. If you feel that this ticket should be included in the next Sage release at the soonest please set its milestone to the next release milestone (sage-8.9).

comment:4 Changed 3 years ago by Frédéric Chapoton

Authors: Frédéric Chapoton
Branch: u/chapoton/27989
Commit: c1c09b8bcb84254c26743ca69579346a89cdffbe
Status: newneeds_review

done, please review


New commits:

c1c09b8trac 27989 purge _l_action and _r_action

comment:5 Changed 3 years ago by Kwankyu Lee

If you see the original discussion:

https://groups.google.com/forum/#!topic/sage-devel/q_672YMI4m4

the issue is not just to remove _l_action_ and _r_action_, but to document how to implement coercion for action properly using _act_on_ and _acted_upon_, instead of now non-existing _l_action_ and _r_action_

comment:6 Changed 3 years ago by Kwankyu Lee

Status: needs_reviewneeds_work

comment:7 Changed 3 years ago by Frédéric Chapoton

oh, well. This can be done in 2 different tickets, I think.

comment:8 Changed 3 years ago by Dima Pasechnik

Status: needs_workneeds_review

I have opened a separate ticket for the documentation: #28017

comment:9 Changed 3 years ago by Frédéric Chapoton

so, is this a positive review ?

comment:10 Changed 3 years ago by Dima Pasechnik

no, I still have to test it.

comment:11 Changed 3 years ago by Dima Pasechnik

Reviewers: Dima Pasechnik
Status: needs_reviewpositive_review

looks and works well, thanks.

comment:12 Changed 3 years ago by Kwankyu Lee

I suggest to merge this ticket with #28017, to which I just pushed a patch. Then Frédéric and I could be coauthors for the patch. And then we can close this ticket as invalid.

comment:13 Changed 3 years ago by Frédéric Chapoton

There is no need to merge. Let us just do one thing after the other.

comment:14 Changed 3 years ago by Dima Pasechnik

I don't see a problem to continue on #28017, which I am currently testing etc.

comment:15 Changed 3 years ago by Frédéric Chapoton

Milestone: sage-8.9

comment:16 Changed 3 years ago by Volker Braun

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