Skip to content
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

issue #66 Add Kata Diamond #70

Merged
merged 2 commits into from
Oct 18, 2020
Merged

issue #66 Add Kata Diamond #70

merged 2 commits into from
Oct 18, 2020

Conversation

farzado
Copy link
Contributor

@farzado farzado commented Oct 17, 2020

  • Generate diamond string based on a given wide point character that is in range of A-Z.
  • Used TDD approach by adding unit tests first.

Resolves #66

@farzado
issue fengyuanyang#66 Add Kata Diamond
f2d056d 
* Generate diamond string based on a given wide point character that is in range of A-Z.
* Used TDD approach by adding unit tests first.

Resolves fengyuanyang#66
@farzado
Copy link
Contributor Author

farzado commented Oct 18, 2020

@fengyuanyang I've updated the PR, could you pls have a look?

fengyuanyang
fengyuanyang reviewed Oct 18, 2020
View reviewed changes
Copy link
Owner

@fengyuanyang fengyuanyang left a comment

Choose a reason for hiding this comment

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

@fengyuanyang I've updated the PR, could you pls have a look?

I didn't see changes, there is only one commit in this pr for now

src/main/java/com/ordestiny/tdd/kata/Diamond.java Outdated Show resolved Hide resolved
src/main/java/com/ordestiny/tdd/kata/Diamond.java Outdated Show resolved Hide resolved
src/test/java/com/ordestiny/tdd/kata/DiamondTest.java Outdated Show resolved Hide resolved
src/test/java/com/ordestiny/tdd/kata/DiamondTest.java Outdated Show resolved Hide resolved
src/main/java/com/ordestiny/tdd/kata/Diamond.java Outdated Show resolved Hide resolved
@januslinhc
Copy link
Contributor

@fengyuanyang I've updated the PR, could you pls have a look?

I didn't see changes, there is only one commit in this pr for now

Becoz he replaced his original commit 😂

@farzado
issue fengyuanyang#66 Update indentation and unit test structure
08b953f 
* Updated indentations in both Diamond and DiamondTest classes
* Modified a unit test's structure by adding assume, act, and assertion comments for better clarity

Resolves fengyuanyang#66
@januslinhc
Copy link
Contributor

I have no idea why he need force push😂

@farzado
Copy link
Contributor Author

farzado commented Oct 18, 2020 •
edited
Loading

I have no idea why he need force push😂

Sorry if it has confused you, some people prefer to have a clean linear git history and to squash all commits into one, that's why I used force push.

@januslinhc
Copy link
Contributor

I have no idea why he need force push😂

Sorry if it has confused you, some people prefer to have a clean linear git history and to squash all commits into one, that's why I used force push.

I see. That's why @fengyuanyang asked why some commits lost

@januslinhc
Copy link
Contributor

I have no idea why he need force push😂

Sorry if it has confused you, some people prefer to have a clean linear git history and to squash all commits into one, that's why I used force push.

I see. That's why @fengyuanyang asked why some commits lost

For me, it's fine if the git history is dirty since you are working in your feature sub-branch. If we commit our changes as squash, it's hard for us to revert in case we need to

@fengyuanyang fengyuanyang added the hacktoberfest-accepted welcome to hacktoberfest- accepted label Oct 18, 2020
@fengyuanyang
Copy link
Owner

oh , that's why !! Thanks for your explanation!!
It seems difficult for reviewer to see what has changed with force push .
It looks good to me now ,
Thanks @januslinhc and @farzado

@fengyuanyang fengyuanyang merged commit dd04d54 into fengyuanyang:main Oct 18, 2020
@farzado farzado deleted the issue--66-add-kata-diamond branch October 18, 2020 23:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@fengyuanyang fengyuanyang fengyuanyang left review comments

@januslinhc januslinhc januslinhc approved these changes

Assignees
No one assigned
Labels
hacktoberfest-accepted welcome to hacktoberfest- accepted
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[TDD] Kata - Diamond
3 participants
@farzado @januslinhc @fengyuanyang