Closed prisae closed 2 years ago
Merging #87 (8b29c5d) into main (999d9ca) will decrease coverage by
0.20%
. The diff coverage is80.00%
.
@@ Coverage Diff @@
## main #87 +/- ##
==========================================
- Coverage 87.15% 86.95% -0.21%
==========================================
Files 4 4
Lines 366 368 +2
==========================================
+ Hits 319 320 +1
- Misses 47 48 +1
Looking at this, I have second thoughts. I think that is a left-over from my old code snippet. Currently it adjusts the spacing to anything between 18 (shortest/default) to 40 (longest) characters, as is shown in the following screenshot with annotations (I think this is partly a leftover because first I had version names on the left and package name on the right. Then, with packages including git hashes etc into the dev-numbers, I had to move the spacing).
My suggestion is to simply centre it and have it the same all the time. What do you think @banesullivan?
Hence, I would replace
# Get length of longest package: min of 18 and max of 40
if self._packages:
row_width = min(40, max(18, len(max(self._packages.keys(), key=len))))
else:
row_width = 40
simply with
row_width = self.text_width // 2 - 2
Any objections to that?
Or maybe
row_width = self.text_width // 3
That would, by the default 80 textwidth, give a column width of 26, which looks nice and should accommodate 99.9% of package names...
Here is how the two suggestions look like:
I leave the discussion about the general layout for another day. I just changed the width from 40 to 18 if there is no package at all; this is the status quo for "regular", short-named packages. As such this PR does really only one thing, fix this bug. I'll merge if all tests pass.
Currently, the
Report()
fails if there is an empty package list, which you get if you callThis PR fixes this, so it works just nicely (it always worked with the
_repr_html
, there was no issue there).Example output (with the fix):
This came up in the introduction of the
--no-opt
-flag for the CLI in #86 - but I thought I'd open a separate PR, as this bug is not really related to the CLI.