Skip to content

migrate FormatStringBuffer to FormatStringBuilder#1150

Merged
johnmay merged 2 commits intocdk:mainfrom
uli-f:migrate-formatstringbuffer-to-stringbuilder
Feb 17, 2025
Merged

migrate FormatStringBuffer to FormatStringBuilder#1150
johnmay merged 2 commits intocdk:mainfrom
uli-f:migrate-formatstringbuffer-to-stringbuilder

Conversation

@uli-f
Copy link
Copy Markdown
Member

@uli-f uli-f commented Feb 9, 2025

  • replace StringBuffer with StringBuilder for improved performance
  • update dependencies and related test classes accordingly
  • simplified code and removed code smells

…r with StringBuilder for improved performance; update dependencies and related test classes accordingly; simplified code and removed code smells
@uli-f
Copy link
Copy Markdown
Member Author

uli-f commented Feb 9, 2025

I opened a new PR for this instead of continuing with Egon's #1135. It seemed easier as most of Egon's initial changes need to be reversed. I also might have needed GitHub permissions to Egon's fork of cdk 🤷🏼

So please close #1135 and merge this one 😃

Copy link
Copy Markdown
Member

@egonw egonw left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would suggest to not to rename the class. Even the internal variable name (buffer) does not have to be changed. It only makes the patch much larger. But if @johnmay is okay with it, I will also not block the PR because of that. And I also feel the pain of the work done on the renaming.

Comment thread base/standard/src/main/java/org/openscience/cdk/tools/FormatStringBuilder.java Outdated
… make FormatStringBuilder package-private; update constructors and methods to package-private; remove unused imports for FormatStringBuilder in related classes
@uli-f
Copy link
Copy Markdown
Member Author

uli-f commented Feb 17, 2025

@johnmay Moved FormatStringBuilder to module io and package io as discussed.

@johnmay johnmay merged commit 132856b into cdk:main Feb 17, 2025
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.

3 participants