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: |
Description (last modified by )
Refactor IntegerMulAction
in preparation for #24247:
- Add an abstract base class
IntegerAction
which will also be used to implementIntegerPowAction
in #24247.
- Add a new helper function
parent_is_integers
to check if some type/parent represents the integers.
- Instead of the hacks in
discover_action
andverify_action
involving an explicitIntegerMulAction
check, return anIntegerMulAction
for anything satisfyingparent_is_integers()
.
Change History (12)
comment:1 Changed 3 years ago by
- Branch set to u/jdemeyer/ticket/24467
comment:2 Changed 3 years ago by
- Commit set to 85cabe71cb8a3f034b99a71e65c5329b79c2a8b7
- Status changed from new to needs_review
comment:3 Changed 3 years ago by
- Commit changed from 85cabe71cb8a3f034b99a71e65c5329b79c2a8b7 to a2d60efa250eb7806f67f313738d67b3288577d3
Branch pushed to git repo; I updated commit sha1. New commits:
a2d60ef | Also verify IntegerAction instances
|
comment:4 Changed 3 years ago by
- Description modified (diff)
comment:5 follow-up: ↓ 6 Changed 3 years ago by
- 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: ↓ 9 Changed 3 years ago by
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
- Commit changed from a2d60efa250eb7806f67f313738d67b3288577d3 to a35986207005594e923ad97e12cb468d0885dfb4
Branch pushed to git repo; I updated commit sha1. New commits:
a359862 | Use \cdot instead of ·
|
comment:8 Changed 3 years ago by
- Status changed from needs_review to positive_review
comment:9 in reply to: ↑ 6 ; follow-up: ↓ 10 Changed 3 years ago by
Replying to jdemeyer:
Replying to tscrim:
The
·
is not ascii nor latexIs 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
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
I see. I wasn't aware of that. Thank you.
comment:12 Changed 3 years ago by
- Branch changed from u/jdemeyer/ticket/24467 to a35986207005594e923ad97e12cb468d0885dfb4
- Resolution set to fixed
- Status changed from positive_review to closed
New commits:
Refactor IntegerMulAction