GeezerVison

The World Wide Web, restored to 800 pixels of glory.

GeezerVison is showing github.com — get rid of needless string allocations by llogiq · Pull Request #7773 · feldera/feldera

Conversation

llogiq commented Nov 1, 2026

Copy link

Copy Markdown

Contributor

This is a very small behavior-neutral change to use write!(..) instead of push_str(format!(..)) in all relevant code. There is also one change that removes a branch for getting a usize for a bool, some places where I changed format!(_) invocations to using embedded syntax, and one point where I use the .midpoint(_) method instead of using the manual implementation that might overflow (although that is probably not going to happen in practice).

All tests run unchanged.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

llogiq and others added 2 commits

November 1, 2026 16:27


          get rid of needless string allocations

19e859b


          [ci] apply automatic fixes

a8d5263

Signed-off-by: feldera-bot <[email protected]>

mythical-fred-oss Bot reviewed

Nov 1, 2026

View reviewed changes

Comment thread

crates/pipeline-manager/src/api/endpoints/pipeline_interaction/support_bundle.rs

		));
		out.push_str(&format!(
		"Pipeline Status: {} (since {})\n",
		let _ = writeln!(&mut out, "Pipeline: {}", meta.pipeline.name);

mythical-fred-oss Bot Nov 1, 2026

Copy link

Copy Markdown

Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

bug (high, CI): cargo clippy -p pipeline-manager -- -D warnings fails with 7 × clippy::write_with_newline in this function (lines 210, 211, 216, 223, 237, 243, 249). Use writeln! and drop the trailing \n, e.g.

Suggested change

	let _ = writeln!(&mut out, "Pipeline: {}", meta.pipeline.name);
	let _ = writeln!(&mut out, "Pipeline: {}", meta.pipeline.name);

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

mythical-fred-oss Bot reviewed

Nov 1, 2026

View reviewed changes

Comment thread

crates/feldera-types/src/config/ignored.rs

		s.push_str(key);
		}
		Segment::Index(index) => s.push_str(&format!("[{index}]")),
		Segment::Index(index) => {

mythical-fred-oss Bot Nov 1, 2026

Copy link

Copy Markdown

Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

bug (medium, CI): this doesn't pass cargo fmt --check. rustfmt wants:

Suggested change

	Segment::Index(index) => {
	Segment::Index(index) => {
	let _ = write!(&mut s, "[{index}]");
	}

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

mythical-fred-oss Bot commented Nov 1, 2026

Copy link

Copy Markdown

Contributor

This refactor doesn't change behavior, and I checked that the pipeline_logs.rs edit is equivalent: display_name already fell back to "N/A", so the old if was always true. The pre-commit gates fail, though (see inline comments).

Check	Result
`cargo clippy --no-deps -p feldera-types -p pipeline-manager -- -D warnings`	❌ 7 × `write_with_newline` in `support_bundle.rs`
`rustfmt --edition 2024 --check` on changed files	❌ `ignored.rs`
`cargo clippy -p feldera-sqllib -p dbsp -p fda -- -D warnings`	✅
`cargo test -p feldera-types ignored`	✅ 3 passed
`cargo test -p pipeline-manager --lib -- support_bundle pipeline_logs`	✅ 25 passed
`cargo test -p feldera-sqllib flat_variant`	✅

pitfall (medium): please run pre-commit run --files <changed files> before pushing; the merge queue will reject this as it stands.
commit (low): the subject should be capitalized (Remove needless string allocations).

I didn't build feldera-connectors (its change is one line, the same as the others). Existing tests cover the touched paths well enough for a change with no behavior impact.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

gz approved these changes

Nov 1, 2026

View reviewed changes

llogiq added this pull request to the merge queue

Nov 1, 2026

Merged via the queue into main with commit 6693026

Nov 1, 2026

1 check passed

Uh oh!

There was an error while loading. Please reload this page.

llogiq deleted the more-small-stuff branch

November 1, 2026 18:03

Reviewers

mythical-fred-oss[bot]

mythical-fred-oss[bot] left review comments

gz approved these changes

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

3 participants

Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.

You are visitor 01402520 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.