Skip to content

[wip] check validity of branch name #759

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

Closed
wants to merge 10 commits into from

Conversation

franciscod
Copy link

(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 :)

@franciscod
Copy link
Author

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

@extrawurst
Copy link
Collaborator

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

Copy link
Author

@franciscod franciscod left a comment

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
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
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
Copy link
Collaborator

@extrawurst extrawurst left a comment

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
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
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!

@@ -60,8 +67,9 @@ impl Component for CreateBranchComponent {
}

if let Event::Key(e) = ev {
self.validate_input()?;
Copy link
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
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
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
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
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
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
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