Skip to content

eliminate string clone by using std::mem::take in json editor tutorial - #1010

Open
AdamSteinberg1 wants to merge 2 commits into
ratatui:mainfrom
AdamSteinberg1:main
Open

eliminate string clone by using std::mem::take in json editor tutorial#1010
AdamSteinberg1 wants to merge 2 commits into
ratatui:mainfrom
AdamSteinberg1:main

Conversation

@AdamSteinberg1

Copy link
Copy Markdown

I was going through the json editor tutorial and noticed on this page that there was an unnecessary clone. In the code we clone two Strings to insert them into a HashMap. Then we clear the contents of the Strings setting them to String::new(). It's unnecessary to clone when the next thing we do is delete. std::mem::take is a better approach.

@orhun

orhun commented Dec 23, 2025

Copy link
Copy Markdown
Member

The tutorials are meant to be beginner friendly so I'm not sure if std::mem::take aligns with that.

With that being said, I'm not against this change. It's a JSON editor tutorial which can be considered as intermediate level.
It would be nice to get some thoughts from @kdheepak who created the tutorial originally I think.

Group the in-progress key/value pair with the active editing field so
saving can move the draft into the stored pairs without cloning. This
keeps the tutorial state model closer to the UI workflow and removes
invalid combinations of editing state.
@joshka

joshka commented Jun 12, 2026

Copy link
Copy Markdown
Member

I pushed an update directly to this PR branch.

I think this shape is better than only replacing clone-and-clear with mem::take(). The old state has several fields (key_input, value_input, and currently_editing) that have to be kept in lock step, so grouping the draft key/value plus focus into a single EditingPair makes the model harder to put into an inconsistent state. It still gets the ownership benefit of moving the completed draft into the map without cloning, and the tutorial narrative now explains that state shape directly.

@joshka

joshka commented Jun 12, 2026

Copy link
Copy Markdown
Member
Screenshot 2026-06-12 at 04 25 45

Weird

@orhun

orhun commented Jun 12, 2026

Copy link
Copy Markdown
Member

Yeah, those CI jobs are stuck for a while now. We can try deleting the cache manually and rerunning them probably

@joshka

joshka commented Jun 12, 2026

Copy link
Copy Markdown
Member

didn't seem to help - maybe something in the pnpm formatting broke.

@joshka

joshka commented Jun 12, 2026

Copy link
Copy Markdown
Member

will take another look later.

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