Skip to content

Test listlabels #331

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

jamillambert
Copy link
Collaborator

listlabels was untested and had a model with no bitcoin types.

Add a test and remove the model.

Update verify, and types.

tcharding
tcharding previously approved these changes Aug 14, 2025
Copy link
Member

@tcharding tcharding left a comment

Choose a reason for hiding this comment

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

ACK ebaa961


let json: ListLabels = node.client.list_labels().expect("listlabels");

assert!(json.0.contains(&label.to_string()));
Copy link
Member

Choose a reason for hiding this comment

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

I'm going to ack/merge this but in case it needs a rebase this line is a bit off. The label var is of type &str. This line allocates memory, creates an String, then immediately dereferences it to get an &str. So we can just pass in label.

Copy link
Member

Choose a reason for hiding this comment

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

Ha I merged in order of PR number, got all the way to here!

Copy link
Collaborator Author

@jamillambert jamillambert Aug 15, 2025

Choose a reason for hiding this comment

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

The label var is of type &str. This line allocates memory, creates an String, then immediately dereferences it to get an &str. So we can just pass in label.

So... I had to change it to any() with a closure. With assert!(json.0.contains(label)); I get the error expected `&String`, found `&str` which is why I changed it to the above originally. I didn't know that you could compare a &String to &str with the == in the closure but not with contains().

`listlabel` was untested and had a model with no bitcoin types.

Add a test and remove the model.

Update verify, and types.
@jamillambert
Copy link
Collaborator Author

I'm glad I had to fix this, the &label.to_string() did annoy me a little, now I know why it was needed and how not to do it.

Also first rebase with jj, had to do it twice, first time messed up the commands but going back with jj op restore worked a charm. Was easy in the end, and it supports using vscode as the merge editor.

i.e. I made the change and rebased to fix merge conflicts.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants