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.

Sign up for GitHub

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

Jump to bottom

Make builder pattern more practical #90

Merged

simonsan merged 10 commits into rust-unofficial:master from tgotwig:patch-1

Jan 3, 2021

Contributor

tgotwig commented Nov 23, 2019 •

edited

Loading

https://github.com/TGotwig/patterns/blob/patch-1/patterns/builder.md

nesteruk commented Dec 24, 2019

It is confusing to have bar: String in both Foo and FooBuilder — consider renaming one of the bars to something else.

tgotwig force-pushed the patch-1 branch from 7946431 to 4c27a40 Compare

December 26, 2019 13:30

Contributor Author

tgotwig commented Dec 26, 2019

@nesteruk Changed this and a little bit more 🙂

lambda-fairy previously requested changes

View reviewed changes

Collaborator

lambda-fairy left a comment •

edited

Loading

LGTM, modulo the comment below

patterns/builder.md Show resolved Hide resolved

Contributor

pickfire commented Jul 14, 2020

This example does not seemed to be explaining much, maybe we could have a real-life example or an explanation on why it needs to be constructed this way rather than just specifying the struct directly?

tgotwig force-pushed the patch-1 branch 2 times, most recently from 5460fdc to 60cd1b0 Compare

October 30, 2020 13:30

Contributor Author

tgotwig commented Oct 30, 2020

@pickfire Hmm, changed it a little bit up.
I think that runnable code is better than not runnable code and the comments should explain enough 🧐

pickfire reviewed

View reviewed changes

patterns/builder.md Outdated Show resolved Hide resolved

pickfire reviewed

View reviewed changes

Contributor

pickfire left a comment

The indentation seemed off, should be 4 spaces rather than 2 spaces.

pickfire reviewed

View reviewed changes

patterns/builder.md Outdated Show resolved Hide resolved

pickfire reviewed

View reviewed changes

patterns/builder.md Outdated Show resolved Hide resolved

pickfire reviewed

View reviewed changes

patterns/builder.md Outdated Show resolved Hide resolved

pickfire reviewed

View reviewed changes

patterns/builder.md Outdated Show resolved Hide resolved


          👷‍♀️ Make builder pattern more practical

54e2e8a

👷‍♀️ Replace fmt::Display by #[derive(Debug)]


👷‍♀️ Rename FooBuilder to just Builder


👷 Set type for Foo-instance


👷 Change finish(...) to build(...)


👷 Rename FooBuilder artifacts to Builder


👷 Change indentation from 2 spaces to 4 spaces


👷 Rename builder_bar to bar


👷 Change finish(...) to build(...)

tgotwig force-pushed the patch-1 branch from fe95d41 to 54e2e8a Compare

October 31, 2020 13:58

tgotwig added 2 commits

October 31, 2020 15:10


          👷‍♀️ Rename Builder back to FooBuilder

c714f7a


          👷 Compare Foo-objects by PartialEq-trait

28c6a2b

pickfire reviewed

View reviewed changes

patterns/builder.md Outdated Show resolved Hide resolved

pickfire reviewed

View reviewed changes

patterns/builder.md Outdated Show resolved Hide resolved

pickfire reviewed

View reviewed changes

patterns/builder.md Outdated Show resolved Hide resolved

tgotwig added 2 commits

November 1, 2020 14:10


          👷 Derive from PartialEq


          👷 Replace all str by String

d2ac988

pickfire approved these changes

View reviewed changes

Contributor

pickfire left a comment

Looks good to me. I think it would be good to have one of the maintainer to check this though.

simonsan added C-enhancement S-review labels

Collaborator

simonsan commented Jan 1, 2021

@tgotwig Hey, thank you for the PR. Could you please rebase on master so the CI is checking this through while we get to reviewing this PR? Thank you ;-)

Contributor Author

tgotwig commented Jan 1, 2021

@simonsan Sure, will do! 😃


          Merge branch 'master' into patch-1

a0cc8a7

Collaborator

simonsan commented Jan 1, 2021

@simonsan Sure, will do! 😃

Sorry, haven't seen that there is no addition to summary or anything needed. So I could do it myself. ;-) It's fine then.

simonsan requested a review from lambda-fairy

January 1, 2021 14:51

MarcoIeni dismissed lambda-fairy’s stale review

January 1, 2021 20:51

I am dismissing lambda-fairy review because it requested changes on something that was resolved. If I don't do this we can't merge this. If I am making a mistake please tell me!

simonsan added this to Review in progress in Content

simonsan and others added 4 commits

January 3, 2021 10:12


          Merge branch 'master' into patch-1

578cad2


          Merge branch 'master' into patch-1

25d151e


          Change back to FooBuilder

903cb52


          Add ignore

8f1add3

simonsan requested review from MarcoIeni and removed request for lambda-fairy

January 3, 2021 10:15

Collaborator

simonsan commented Jan 3, 2021 •

edited

Loading

@MarcoIeni You also want to look over it again?

Content automation moved this from Review in progress to Reviewer approved

MarcoIeni approved these changes

View reviewed changes

simonsan merged commit 6bab5f3 into rust-unofficial:master

Content automation moved this from Reviewer approved to Done

Collaborator

simonsan commented Jan 3, 2021

Thanks! :-)

Contributor Author

tgotwig commented Jan 3, 2021

Cool! 😊🚀

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