Conversation
When rendering a scorecard for command line programs, the entry point will frequently have spaces (e.g., "bbi nonmem run local").
Under strict=TRUE, wrap_text first tries to get the string under the
specified width by splitting at the wrap_sym characters and then falls
back to breaking the string at whatever positions are necessary to get
the string under the specified width.
If the package uses camelCase for their functions, it's more readable
to split before the capital letter. For example,
diffPrevious
Revisions
looks better than
diffPreviousRevision
s
Try splitting at mid-string capital letters before breaking the string
at any character.
When preparing to score Torsten, a few entry points are running into the next column (e.g., pmx_solve_group_adams). Ideally, we'd be able to avoid these problems by choosing a width where we know the column can fit any string with fewer characters. For example, with the current variable-width font, W is the widest letter. Using long runs of Ws as dummy values for each column, I was able to get the values to fit by specifying widths of 9 (instead of 24) and 17 (instead of 27) for the non-test columns and test column, respectively. Although those values should prevents any text from running over, they unfortunately lead to excessive and very bad looking breaks for realistic names. At least until we come up with a better approach, just reduce the width to make the Torsten matrix not have values that run over the column boundary.
barrettk
left a comment
barrettk
left a comment
There was a problem hiding this comment.
This looks good and thanks for the detailed commit messages.
I like the capital letter fallback. The letters example you sent offline makes me wonder if we should consider exposing the width argument via a ... argument at some point so that this could be adjusted for specific packages in the MPN build toolchain - would that be helpful?
Additionally, the letters seem to go off the page for the test files:
- Did you consider if adjusting that wrapping too given the observations you mention in 4e488f6?
You say "MPN build toolchain"; no, I don't think exposing an argument or worrying about widths is something that we should be concerned with in the context of MPN. However, for MetrumRG scorecards, we do want to make sure nothing runs over. Seth raised a similar point offline about whether it'd be good to expose an argument. I'm not against it, though I think it'd take more thought and work to make useful. Exposing the width as an argument or option would make it possible to resolve a runoff issue without an mpn.release and even possible to have different widths per package. But that would then involve exposing those through our internal driver script, and, if we were planning on using different widths per package (rather than using the argument for a temporary fix before a adjusting mpn.scorecard), we would need to store those configurations somewhere so the caller didn't have to remember/know to specify some width value. So, perhaps, but at this point I think we should just adjust the width and revisit the idea if we hit into this problem again.
Yes, the letter example shows cases that go off for all columns. The point I'm making with that letter example is that we'd need to adjust to excessively small widths to get nothing to run off. From 4e488f6: Test files aren't running off in any known examples, so I don't see a reason to adjust the column width (just like I don't think we should farther adjust the non-test columns even though some letters still run off). |
|
Another thing worth considering at some point: switching the matrix table and perhaps other tables to use fixed-width font. That should allow us to determine a single column/character width that is sufficient to avoid running over. Aside from cosmetics of getting that font change accepted, an open question is whether the determined value would lead to reasonable looking wrapping as opposed to the excessive wrapping that's needed to get the variable-width letter example (in particular a string of Ws) to not run off. |
barrettk
left a comment
barrettk
left a comment
There was a problem hiding this comment.
Looks good! Thanks for the replies
|
@barrettk Thanks for the review. |
2 participants
When preparing to use mpn.scorecard for Torsten, some of entry points in the matrix ran into the next column. The main purpose of this PR is to avoid that by decreasing the specified column width [*]. (See last commit for information.)
In addition, this PR makes two more wrapping tweaks:
for strict mode, prefer breaking at mid-string capital letters
This relates to the decreased column width. With the width change, I noticed that review's
diffPreviousRevisionswas getting unpleasantly wrapped so that the finalswas on the second line. By considering mid-string capital letters, the break instead happens beforeRevisions.add space to the default
wrap_symvalueThis makes it more likely that CLI entry points find a good break point, which becomes more relevant as we decrease the widths. (However, none of the CLIs that we currently score have entry points that exceed the new width, so it doesn't matter at this point.)
[*] note added after this was merged, to hopefully reduce chance of confusion when we revisit this: "column width" is confusing word choice. It's referring to the number of characters at which a value is wrapped (i.e. the width value passed to
wrap_text). This PR does not decrease the width of the table columns themselves (which would make it more likely a value would run outside the column boundary!).