Skip to content

Fixed line width calculation for helm source - #270

Closed
ryan-c-scott wants to merge 3 commits into
alphapapa:stable/0.6from
ryan-c-scott:fix-long-helm-entries
Closed

Fixed line width calculation for helm source#270
ryan-c-scott wants to merge 3 commits into
alphapapa:stable/0.6from
ryan-c-scott:fix-long-helm-entries

Conversation

@ryan-c-scott

Copy link
Copy Markdown

.Was resulting in negative values on long entries and the resulting
error was being suppressed by `ignore-errors' in the source

alphapapa and others added 3 commits March 1, 2022 02:25
Fixes alphapapa#248.  Closes alphapapa#249.

Thanks to Maikol Solis (@maikol-solis) for reporting, and to him and
Ihor Radchenko (@yantar92) for helping plan the fix.
.Was resulting in negative values on long entries and the resulting
error was being suppressed by `ignore-errors' in the source
@alphapapa

Copy link
Copy Markdown
Owner

Thanks for working on this.

Looking at the change, I'm not sure if I understand exactly what's happening, or whether this would be the best way to fix it. If the width variable is set to 0 for certain entries, it might result in a string that isn't useful or readable. Can you show me an example of an entry that causes this problem, and what the result is after the fix is applied?

@ryan-c-scott

ryan-c-scott commented Mar 21, 2022

Copy link
Copy Markdown
Author

Thanks for working on this.

Looking at the change, I'm not sure if I understand exactly what's happening, or whether this would be the best way to fix it. If the width variable is set to 0 for certain entries, it might result in a string that isn't useful or readable. Can you show me an example of an entry that causes this problem, and what the result is after the fix is applied?

I'll try to get a more concrete example for you soon, but the current behavior causes any source that would show one of these long entries to show absolutely nothing at all.

The behavior when that value gets calculated as 0 is just that it effectively uses the whole original string.

Regardless long strings causing that value to be negative and then throw an invisible error will need to be addressed for usability.

@alphapapa

Copy link
Copy Markdown
Owner

Ok, thanks.

If you would, since this is a bug fix, please rebase the patch onto the stable/0.6 branch so I can make a new stable release with this fix. If you want to update the changelog as well, that's fine, or I can handle that (either way, I'll update the generated info manual).

@ryan-c-scott
ryan-c-scott changed the base branch from master to stable/0.6 March 22, 2022 04:00
@ryan-c-scott

Copy link
Copy Markdown
Author

Created new [cleaner, saner] PR, #271 .

@ryan-c-scott
ryan-c-scott deleted the fix-long-helm-entries branch March 22, 2022 04:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants