Skip to content

[wip] check validity of branch name - #759

Closed
franciscod wants to merge 10 commits into
gitui-org:masterfrom
franciscod:wip_check_branch_name
Closed

franciscod wants to merge 10 commits into
gitui-org:masterfrom
franciscod:wip_check_branch_name

Conversation

@franciscod

Copy link
Copy Markdown

(Okay, here we go, rust PR number two 💃)

I'm definitely certain that this is ugly code. It works, it's probably correct, but it's not pretty. I don't know what I should do to improve it.

Feedback would be greatly appreciated :)

Comment thread asyncgit/Cargo.toml Resolved
@franciscod

Copy link
Copy Markdown
Author

that clippy thing sure looks handy. will run it locally and use it to improve this :)

@extrawurst

Copy link
Copy Markdown
Collaborator

We need a git2 release. We can’t depend on a revision

Comment thread asyncgit/src/sync/branch/mod.rs Resolved
Comment thread src/components/create_branch.rs Resolved
Comment thread src/components/create_branch.rs Resolved
Comment thread src/components/create_branch.rs Resolved
Comment thread src/components/create_branch.rs Resolved

@franciscod franciscod left a comment

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

i don't know how I started this review and I don't know how to cancel it

@extrawurst

Copy link
Copy Markdown
Collaborator

i don't know how I started this review and I don't know how to cancel it

what do you mean?

@franciscod

Copy link
Copy Markdown
Author

i don't know how I started this review and I don't know how to cancel it

what do you mean?

I accidentally started some kind of review on the github UI and couldn't cancel it, only add a comment

@franciscod
franciscod requested a review from extrawurst June 3, 2021 20:17

@extrawurst extrawurst left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

aside this small improvement we mostly need to wait for a git2-rs release. you can ask Alex in your PR to cut a release

fn validate_input(&mut self) -> Result<()> {
let branch_name = self.input.get_text().as_str();

self.already_exists = false;

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

since we only show either invalid_name or already exists (which is ok) we also only need to store the validity in an enum. this also means we do not need to test the for the other if the first check already fails.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

ah, makes sense! will do. thanks!

}

if let Event::Key(e) = ev {
self.validate_input()?;

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

does this even work? IMO every key consumed by self.input will not even go here

@extrawurst

Copy link
Copy Markdown
Collaborator

did you test your latest version of this even?

Screenshot 2021-06-04 at 23 05 18

self.already_exists = true;
}
}
self.invalid_name = sync::branch_name_is_valid(branch_name)?;

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

this cannot work, this is inverted

let branch_name = self.input.get_text().as_str();

self.already_exists = false;
self.invalid_name = false;

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

this assignment is useless

@franciscod

Copy link
Copy Markdown
Author

sorry for the rushed commit. I was overconfident that it would work. I'll take my time and test every corner of it before requesting a review again.

@extrawurst

Copy link
Copy Markdown
Collaborator

@franciscod I will mark this as draft in the meantime, please ping me when you want me to check it out again 👍

@extrawurst
extrawurst marked this pull request as draft June 9, 2021 21:54
@extrawurst

Copy link
Copy Markdown
Collaborator

@franciscod do you still plan to finish this?

@extrawurst extrawurst closed this Aug 15, 2021
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.

2 participants