Repository navigation
Commit c20408d
authored
refactor: consistent printline() across pose and quaternion classes (#200)
## What
Consistent `printline()` behaviour across `SO2`/`SE2`/`SO3`/`SE3`/
`Quaternion`/`UnitQuaternion`:
- Adds `Quaternion.printline()` (previously only the pose classes had
it).
- Splits the string-building logic out of `trprint`/`trprint2`/`qprint`
into
dedicated `tr2str`/`tr2str2`/`q2str`-style functions (the latter two
already existed; `tr2str`/`tr2str2` are new), so "give me the formatted
string" and "print the formatted string" are two separate, clearly-named
functions instead of one function trying to do both via a `file` flag.
- Fixes the original bug this was written for:
`contextlib.redirect_stdout()`
didn't work with the old code, because `file=sys.stdout` as a default
argument value is evaluated once at function-definition time and freezes
whatever `sys.stdout` was *then* - it doesn't see a later redirect.
Fixed
by using a `file=False` sentinel, resolved to `None` inside the function
body (so `print()` looks up the *current* `sys.stdout` at call time).
## Bugs found while packaging this up for review
This started as old WIP work from a local branch, never previously
reviewed or merged. Testing it before submission turned up three real
bugs,
all fixed here:
1. **`trprint()`/`qprint()` silently shifted `file` into the 2nd
positional
argument.** The original, already-published signatures had `orient`
(`trprint`) / `delim` (`qprint`) there. `trprint(T, 'angvec')` or this
library's *own documented* `x.printline('angvec')` example put
`'angvec'` into `file` instead, crashing with `AttributeError: 'str'
object has no attribute 'write'`. Fixed by restoring the original
explicit parameter order for both (`trprint2` was unaffected - its
parameter order happened to already put `file` in the same slot).
2. **`trprint2()` captured `label` but never forwarded it** to
`tr2str2()`
- `trprint2(T, label='T')` silently dropped the label from the output.
3. **All three stopped returning the formatted string**, only printing
it -
breaking the existing, still-supported `file=None` → "return the string
instead of printing" usage that a couple of existing tests rely on
(`test_transforms3d.py::test_print`,
`test_transforms2d.py::test_print2`).
While fixing (3), also formally deprecated that `file=None` pattern
rather
than silently perpetuating it: each function now has a real
`tr2str()`/`tr2str2()`/`q2str()` sibling that returns the string
directly,
so `file=None` still works exactly as before (returns the string,
doesn't
print) but now raises a `DeprecationWarning` and says so explicitly in
the
docstring via `.. deprecated::`, pointing at the replacement function.
All three (`trprint`/`trprint2`/`qprint`) now follow one consistent
shape:
build the string via the `*2str`/`q2str` sibling, always return it,
print
only when `file is not None`.
## Testing
- Verified `file=None` still returns the correct string with a
`DeprecationWarning` and no printing, for all three functions.
- Verified the default path both prints (via
`contextlib.redirect_stdout`,
confirming the original bug is actually fixed) and returns the same
string.
- Verified positional-arg calls (`trprint(T, 'angvec')`,
`x.printline('angvec')`) work correctly again.
- Full suite: 338 passed, 4 skipped.
- `black --check` clean at the pinned 23.10.0.
## Note on PR ordering
This PR and #198 (lazy Matplotlib import) both touch adjacent code in
`spatialmath/base/transforms3d.py` (right around `_vec2s`/
`_matplotlib_exists`/`trprint`/`trplot`) - not a conflict with anything
already merged, just two open PRs based on the same `upstream/master`
point. Whichever lands first, the other will likely need a rebase to
resolve a real merge conflict before it can land.1 parent 45ed832 commit c20408d
6 files changed
Lines changed: 238 additions & 57 deletions
File tree
- spatialmath
- base
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
225 | 225 | | |
226 | 226 | | |
227 | 227 | | |
| 228 | + | |
228 | 229 | | |
229 | 230 | | |
230 | 231 | | |
| |||
268 | 269 | | |
269 | 270 | | |
270 | 271 | | |
| 272 | + | |
271 | 273 | | |
272 | 274 | | |
273 | 275 | | |
| |||
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
1188 | 1188 | | |
1189 | 1189 | | |
1190 | 1190 | | |
1191 | | - | |
1192 | | - | |
| 1191 | + | |
| 1192 | + | |
1193 | 1193 | | |
1194 | | - | |
| 1194 | + | |
1195 | 1195 | | |
1196 | 1196 | | |
1197 | 1197 | | |
| |||
1219 | 1219 | | |
1220 | 1220 | | |
1221 | 1221 | | |
| 1222 | + | |
| 1223 | + | |
| 1224 | + | |
| 1225 | + | |
1222 | 1226 | | |
1223 | 1227 | | |
1224 | 1228 | | |
| 1229 | + | |
1225 | 1230 | | |
1226 | 1231 | | |
1227 | 1232 | | |
1228 | 1233 | | |
1229 | 1234 | | |
1230 | | - | |
| 1235 | + | |
| 1236 | + | |
| 1237 | + | |
| 1238 | + | |
| 1239 | + | |
| 1240 | + | |
| 1241 | + | |
1231 | 1242 | | |
1232 | 1243 | | |
1233 | 1244 | | |
| |||
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
18 | 18 | | |
19 | 19 | | |
20 | 20 | | |
| 21 | + | |
21 | 22 | | |
22 | 23 | | |
23 | 24 | | |
| |||
976 | 977 | | |
977 | 978 | | |
978 | 979 | | |
979 | | - | |
| 980 | + | |
980 | 981 | | |
981 | 982 | | |
982 | | - | |
983 | 983 | | |
984 | 984 | | |
985 | 985 | | |
986 | 986 | | |
987 | | - | |
| 987 | + | |
988 | 988 | | |
989 | 989 | | |
990 | 990 | | |
991 | 991 | | |
992 | 992 | | |
993 | | - | |
994 | | - | |
995 | 993 | | |
996 | 994 | | |
997 | 995 | | |
| |||
1002 | 1000 | | |
1003 | 1001 | | |
1004 | 1002 | | |
1005 | | - | |
| 1003 | + | |
1006 | 1004 | | |
1007 | 1005 | | |
1008 | 1006 | | |
1009 | 1007 | | |
1010 | | - | |
| 1008 | + | |
1011 | 1009 | | |
1012 | 1010 | | |
1013 | 1011 | | |
| |||
1016 | 1014 | | |
1017 | 1015 | | |
1018 | 1016 | | |
1019 | | - | |
1020 | | - | |
1021 | | - | |
| 1017 | + | |
| 1018 | + | |
1022 | 1019 | | |
1023 | 1020 | | |
1024 | 1021 | | |
1025 | 1022 | | |
1026 | 1023 | | |
1027 | 1024 | | |
1028 | 1025 | | |
1029 | | - | |
| 1026 | + | |
| 1027 | + | |
| 1028 | + | |
1030 | 1029 | | |
1031 | 1030 | | |
1032 | 1031 | | |
| |||
1045 | 1044 | | |
1046 | 1045 | | |
1047 | 1046 | | |
1048 | | - | |
1049 | | - | |
1050 | 1047 | | |
1051 | 1048 | | |
1052 | 1049 | | |
| |||
1069 | 1066 | | |
1070 | 1067 | | |
1071 | 1068 | | |
| 1069 | + | |
| 1070 | + | |
| 1071 | + | |
| 1072 | + | |
| 1073 | + | |
| 1074 | + | |
| 1075 | + | |
| 1076 | + | |
| 1077 | + | |
| 1078 | + | |
| 1079 | + | |
| 1080 | + | |
| 1081 | + | |
| 1082 | + | |
| 1083 | + | |
| 1084 | + | |
| 1085 | + | |
| 1086 | + | |
| 1087 | + | |
| 1088 | + | |
| 1089 | + | |
| 1090 | + | |
| 1091 | + | |
| 1092 | + | |
| 1093 | + | |
| 1094 | + | |
| 1095 | + | |
| 1096 | + | |
| 1097 | + | |
| 1098 | + | |
| 1099 | + | |
| 1100 | + | |
| 1101 | + | |
| 1102 | + | |
| 1103 | + | |
| 1104 | + | |
| 1105 | + | |
| 1106 | + | |
| 1107 | + | |
| 1108 | + | |
| 1109 | + | |
| 1110 | + | |
| 1111 | + | |
| 1112 | + | |
| 1113 | + | |
| 1114 | + | |
| 1115 | + | |
| 1116 | + | |
| 1117 | + | |
| 1118 | + | |
| 1119 | + | |
| 1120 | + | |
| 1121 | + | |
| 1122 | + | |
| 1123 | + | |
| 1124 | + | |
| 1125 | + | |
| 1126 | + | |
| 1127 | + | |
| 1128 | + | |
| 1129 | + | |
| 1130 | + | |
| 1131 | + | |
| 1132 | + | |
| 1133 | + | |
| 1134 | + | |
| 1135 | + | |
| 1136 | + | |
| 1137 | + | |
| 1138 | + | |
1072 | 1139 | | |
1073 | 1140 | | |
1074 | 1141 | | |
| |||
0 commit comments