Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Don't add html to the toString() of PurchaseRequest #1338

Merged
merged 5 commits into from
Dec 13, 2022

Conversation

libraryaddict
Copy link
Contributor

As far as I can tell, it's meaningless as ListCellRenderer handles the rendering.

Always found it a little irritating when copying a mall search result came up with <html><nobr><font color="gray">Manual of Numberology (1 @ 85,000,000): Cheap Stuff Here --&gt;</font></nobr></html>

Especially when other list cell renderers didn't have this behavior.

So I looked into it, and it appears quite meaningless.

https://github.com/kolmafia/kolmafia/blob/main/src/net/sourceforge/kolmafia/swingui/widget/ListCellRendererFactory.java#L121

…tell, it's meaningless as ListCellRenderer handles it.
@libraryaddict libraryaddict requested a review from a team as a code owner December 11, 2022 05:20
@codecov
Copy link

codecov bot commented Dec 11, 2022

Codecov Report

Merging #1338 (9ccc667) into main (66314c2) will not change coverage.
The diff coverage is n/a.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff            @@
##               main    #1338   +/-   ##
=========================================
  Coverage     33.15%   33.15%           
  Complexity    16589    16589           
=========================================
  Files          1045     1045           
  Lines        162874   162865    -9     
  Branches      35356    35354    -2     
=========================================
- Hits          54008    54005    -3     
+ Misses        99414    99407    -7     
- Partials       9452     9453    +1     
Impacted Files Coverage Δ
.../sourceforge/kolmafia/request/PurchaseRequest.java 26.36% <ø> (+1.99%) ⬆️
...sourceforge/kolmafia/textui/langserver/Script.java 80.85% <0.00%> (-3.20%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 66314c2...9ccc667. Read the comment docs.

@jaadams5
Copy link
Contributor

FWIW the code with the HTML does go back to 2013.

How confident are you that CellRenderers are the only place that this toString() is used? I'm having difficulty answering that and there are definitely cases where HTML in session logs is considered A Good Thing.

I'll approve but someone with more confidence than I about where it is actually used can push.

@midgleyc
Copy link
Member

I can't see anything obvious and I think with our audience we'll know quite quickly if this is having negative effects for people.

Thanks!

@midgleyc midgleyc enabled auto-merge (squash) December 13, 2022 16:10
@midgleyc midgleyc merged commit f22838f into kolmafia:main Dec 13, 2022
@libraryaddict libraryaddict deleted the purchase_tostring branch December 13, 2022 20: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
3 participants