Opened 3 years ago

Closed 3 years ago

#24467 closed enhancement (fixed)

Refactor IntegerMulAction

Reported by: jdemeyer Owned by:
Priority: major Milestone: sage-8.2
Component: coercion Keywords:
Cc: Merged in:
Authors: Jeroen Demeyer Reviewers: Travis Scrimshaw
Report Upstream: N/A Work issues:
Branch: a359862 (Commits, GitHub, GitLab) Commit: a35986207005594e923ad97e12cb468d0885dfb4
Dependencies: Stopgaps:

Status badges

Description (last modified by jdemeyer)

Refactor IntegerMulAction in preparation for #24247:

  1. Add an abstract base class IntegerAction which will also be used to implement IntegerPowAction in #24247.
  1. Add a new helper function parent_is_integers to check if some type/parent represents the integers.
  1. Instead of the hacks in discover_action and verify_action involving an explicit IntegerMulAction check, return an IntegerMulAction for anything satisfying parent_is_integers().

Change History (12)

comment:1 Changed 3 years ago by jdemeyer

  • Branch set to u/jdemeyer/ticket/24467

comment:2 Changed 3 years ago by jdemeyer

  • Commit set to 85cabe71cb8a3f034b99a71e65c5329b79c2a8b7
  • Status changed from new to needs_review

New commits:

85cabe7Refactor IntegerMulAction

comment:3 Changed 3 years ago by git

  • Commit changed from 85cabe71cb8a3f034b99a71e65c5329b79c2a8b7 to a2d60efa250eb7806f67f313738d67b3288577d3

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

a2d60efAlso verify IntegerAction instances

comment:4 Changed 3 years ago by jdemeyer

  • Description modified (diff)

comment:5 follow-up: Changed 3 years ago by tscrim

  • Reviewers set to Travis Scrimshaw

The · is not ascii nor latex: n · a = a + a + ... + a. So if you change that to \cdot (personally, I would also change the ... to \cdots), then you can set a positive review on my behalf.

comment:6 in reply to: ↑ 5 ; follow-up: Changed 3 years ago by jdemeyer

Replying to tscrim:

The · is not ascii nor latex

Is that a problem really? The documentation builds fine and I think that · is more readable than \cdot in text mode.

comment:7 Changed 3 years ago by git

  • Commit changed from a2d60efa250eb7806f67f313738d67b3288577d3 to a35986207005594e923ad97e12cb468d0885dfb4

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

a359862Use \cdot instead of ·

comment:8 Changed 3 years ago by jdemeyer

  • Status changed from needs_review to positive_review

comment:9 in reply to: ↑ 6 ; follow-up: Changed 3 years ago by tscrim

Replying to jdemeyer:

Replying to tscrim:

The · is not ascii nor latex

Is that a problem really? The documentation builds fine and I think that · is more readable than \cdot in text mode.

With the non-ascii characters, the pdf docs tend to have difficulty if the encoding is not declared (in particular, the non-ascii hyphen comes to mind). What we probably should do is add a bunch of the unicode versions of certain latex commands to the console doc replacer since we generally assume terminals support unicode.

Thanks for the change.

comment:10 in reply to: ↑ 9 Changed 3 years ago by jdemeyer

Replying to tscrim:

What we probably should do is add a bunch of the unicode versions of certain latex commands to the console doc replacer since we generally assume terminals support unicode.

No, we do the opposite: replace certain Unicode characters by latex when building the PDF docs. In src/doc/common/conf.py you will find for example

\DeclareUnicodeCharacter{00B7}{\cdot}

saying that · should be replaced by \cdot. So · in docstrings should work just fine.

comment:11 Changed 3 years ago by tscrim

I see. I wasn't aware of that. Thank you.

comment:12 Changed 3 years ago by vbraun

  • Branch changed from u/jdemeyer/ticket/24467 to a35986207005594e923ad97e12cb468d0885dfb4
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.