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.

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

Add toStack to Ordered Primitive Iterable #1068

Closed

Conversation

DineshPurushothamacharya
Copy link
Contributor

Signed-off-by: Dinesh Purushothamacharya [email protected]

Copy link
Contributor

@donraab donraab left a comment

Choose a reason for hiding this comment

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

Thank you for the contribution @DineshPurushothamacharya. I've added comments inline. Please write tests for all of the toStack methods that have been added in the corresponding primitive test .stg files.

@Override
public Mutable<name>Stack toStack()
{
return <name>Stacks.mutable.withAll(this);
Copy link
Contributor

Choose a reason for hiding this comment

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

This can be simplified to just add the single element to the stack as the toList and toSet methods do.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agreed.

@Override
public Mutable<name>Stack toStack()
{
return <name>Stacks.mutable.withAll(this);
Copy link
Contributor

Choose a reason for hiding this comment

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

This can return an empty stack.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure.

@Override
public Mutable<name>Stack toStack()
{
return <name>Stacks.mutable.withAll(this);
Copy link
Contributor

Choose a reason for hiding this comment

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

This can return a stack with a single item added.

@Override
public Mutable<name>Stack toStack()
{
return <name>Stacks.mutable.withAll(this);
Copy link
Contributor

Choose a reason for hiding this comment

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

This should use <name>Stacks.mutable.withAllReversed(this) to be consistent with the object stack.

@Override
public Mutable<name>Stack toStack()
{
return <name>Stacks.mutable.withAll(this);
Copy link
Contributor

Choose a reason for hiding this comment

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

This should be:

        synchronized (this.lock)
        {
            return this.stack.toStack();
        }

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure. I Will updated it.

@Override
public Mutable<name>Stack toStack()
{
return <name>Stacks.mutable.withAll(this);
Copy link
Contributor

Choose a reason for hiding this comment

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

This should be:

        synchronized (this.lock)
        {
            return this.stack.toStack();
        }

@Override
public Mutable<name>Stack toStack()
{
return <name>Stacks.mutable.withAll(this);
Copy link
Contributor

Choose a reason for hiding this comment

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

This should be:

return this.stack.toStack()

DineshPurushothamacharya added a commit to DineshPurushothamacharya/eclipse-collections that referenced this pull request Jun 13, 2021
Signed-off-by: Dinesh Purushothamacharya <[email protected]>
@DineshPurushothamacharya
Copy link
Contributor Author

@donraab I have updated the project with all the review fixes and unit tests for the changes. Please do review it and let me know if any more changes required will be happy to fix them.

@prathasirisha
Copy link
Contributor

@donraab I have updated the project with all the review fixes and unit tests for the changes. Please do review it and let me know if any more changes required will be happy to fix them.

Hi, @DineshPurushothamacharya could you please rebase your PR so one of us can review the changes. Thank you for your contribution!

@donraab
Copy link
Contributor

donraab commented May 9, 2023

Hi @DineshPurushothamacharya, I'd like to help get this PR reviewed and merged. There are a couple of changes to start with since this PR has been open for a couple years.

  1. Update Since tags from 11.0 to 12.0 which will be the next major release
  2. Rollback the changes to the XML files as they are unrelated to this PR
  3. Rebase and squash your commits

Thanks!

@DineshPurushothamacharya
Copy link
Contributor Author

Hi @DineshPurushothamacharya, I'd like to help get this PR reviewed and merged. There are a couple of changes to start with since this PR has been open for a couple years.

  1. Update Since tags from 11.0 to 12.0 which will be the next major release
  2. Rollback the changes to the XML files as they are unrelated to this PR
  3. Rebase and squash your commits

Thanks!

Hello @donraab, sorry for the huge delay on this PR. I have made all the suggested changes, request you to verify it.

Thanks

@donraab
Copy link
Contributor

donraab commented May 15, 2023

Hi @DineshPurushothamacharya, looks like you have some actual issues to fix reported by the checkstyle build. Also, the EclipseFdn ECA check is failing. Do you have a signed ECA on file? If so, I will check and validate what the issue is. Thanks!

@DineshPurushothamacharya
Copy link
Contributor Author

Hi @DineshPurushothamacharya, looks like you have some actual issues to fix reported by the checkstyle build. Also, the EclipseFdn ECA check is failing. Do you have a signed ECA on file? If so, I will check and validate what the issue is. Thanks!

Hi @donraab, I have fixed the checkstyle issue and EclipseFdn ECA. Please request to validate.

@DineshPurushothamacharya
Copy link
Contributor Author

DineshPurushothamacharya commented Jul 22, 2023

@donraab , @prathasirisha could you please review this PR? I would like to close this one.

Copy link
Contributor

@donraab donraab left a comment

Choose a reason for hiding this comment

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

Thank you for the contribution @DineshPurushothamacharya ! I have a couple minor changes requested and after that I will approve and merge.

public void toStack()
{
Mutable<name>Stack stack = this.classUnderTest().toStack();
Assert.assertEquals(stack, <name>Stacks.mutable.with(<(literal.(type))("1")>));
Copy link
Contributor

Choose a reason for hiding this comment

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

The arguments in the assertEquals should be reversed (expected, equals)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have reversed the values for assertEquals.

public void toStack()
{
Mutable<name>Stack stack = <name>Stacks.mutable.withAllReversed(<name>ArrayStack.newStackWith(<["1", "2", "3"]:(literal.(type))(); separator=", ">));
Assert.assertEquals(<name>ArrayStack.newStackWith(<["1", "2", "3"]:(literal.(type))(); separator=", ">).toStack(), stack);
Copy link
Contributor

Choose a reason for hiding this comment

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

The arguments in assertEquals should be reversed (expected, equals)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have reversed the values for assertEquals.

Signed-off-by: Dinesh Purushothamacharya <[email protected]>

Add toStack to Ordered primitive Iterable

Signed-off-by: Dinesh Purushothamacharya <[email protected]>

rebase with master

Signed-off-by: Dinesh Purushothamacharya <[email protected]>

rebase with master

Signed-off-by: Dinesh Purushothamacharya <[email protected]>

Add toStack to Ordered Primitive Iterable

Signed-off-by: Dinesh Purushothamacharya <[email protected]>

Fix checkstyle Issue

Signed-off-by: Dinesh Purushothamacharya <[email protected]>

Add unit tests to support toStack implmentation

Signed-off-by: Dinesh Purushothamacharya <[email protected]>

rebase

rebase

rebase

rebase with master

Signed-off-by: Dinesh Purushothamacharya <[email protected]>

Add toStack to Ordered Primitive Iterable

Signed-off-by: Dinesh Purushothamacharya <[email protected]>

Revert "rebase"

This reverts commit cb766b1.

fix rebase issues

restore xml files

add .gitignore file

Signed-off-by: Dinesh Purushothamacharya <[email protected]>

fix checkstyle issues

Signed-off-by: Dinesh Purushothamacharya <[email protected]>

Fix checkstyle issue

Signed-off-by: Dinesh Purushothamacharya <[email protected]>

fix checkstyle issues

Signed-off-by: Dinesh Purushothamacharya <[email protected]>

Fix assertions
@DineshPurushothamacharya
Copy link
Contributor Author

Thank you for the contribution @DineshPurushothamacharya ! I have a couple minor changes requested and after that I will approve and merge.

@donraab Thanks for pointing the mistakes. I have now fixed them. Please check once again and approve if its looks good to you.

@DineshPurushothamacharya
Copy link
Contributor Author

Hello @donraab could you please approve and merge this PR?

@DineshPurushothamacharya
Copy link
Contributor Author

Hello @donraab, I have fixed the review comments as per your suggestions. Can you please review it and merge this PR?

@donraab
Copy link
Contributor

donraab commented Nov 12, 2023

Hi @DineshPurushothamacharya, I am reviewing now. Thank you for your patience!

@donraab
Copy link
Contributor

donraab commented Nov 16, 2023

Hi @DineshPurushothamacharya, I was hoping to save you the trouble of rebasing your PR and tried using the GitHub Update Branch button for my first time. Unfortunately I did not realize the default behavior of the button is to merge, not rebase. Lesson learned. Would you be able to create a branch, cherry pick your one commit and submit a new PR and make sure it is rebased? I will review/approve and merge the new PR promptly, and close this PR. Thank you, and apologies for the extra work!

@DineshPurushothamacharya
Copy link
Contributor Author

Hi @DineshPurushothamacharya, I was hoping to save you the trouble of rebasing your PR and tried using the GitHub Update Branch button for my first time. Unfortunately I did not realize the default behavior of the button is to merge, not rebase. Lesson learned. Would you be able to create a branch, cherry pick your one commit and submit a new PR and make sure it is rebased? I will review/approve and merge the new PR promptly, and close this PR. Thank you, and apologies for the extra work!

Hi @donraab, Thanks for reviewing it. As mentioned I have rebased the changes onto new branch. Please find the new review. Request you to review this one.
#1528

@donraab donraab closed this Dec 3, 2023
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.

3 participants