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

[core] Add insert & replace child nodes #858

Closed

Conversation

MatiasComercio
Copy link
Contributor

@MatiasComercio MatiasComercio commented Jan 16, 2018

PR Description:
Implements: #693: Add insert& replace operations over nodes

List of changes

  • Update Node interface
    • Add insert method to insert new children in certain indexes
    • Add replace method to replace current children with new ones in certain indexes
    • Rename removeChildAtIndex method to remove with an index parameter
      • Fix test cases to match the new convention
  • Implement insert & replace methods in AbstractNode
  • Add test cases for the new features
  • Leave some TODOs related to the autofix flow
    • NodeEvents will be implemented soon; that's why these TODOs were left in the code

- Update Node interface
  - Add insert method to insert new children in certain indexes
  - Add replace method to replace current children with new ones in certain indexes
  - Rename removeChildAtIndex method to remove with an index parameter
    - Fix test cases to match the new convention
- Implement insert & replace methods in AbstractNode
- Add test cases for the new features
- Leave some TODOs related to the autofix flow
  - NodeEvents will be implemented soon; that's why these TODOs were left in the code
@jsotuyod jsotuyod self-assigned this Jan 17, 2018
@jsotuyod jsotuyod added this to the 6.1.0 milestone Jan 17, 2018
@oowekyala oowekyala added the in:autofixes Affects the autofixes framework label Jan 23, 2018
protected GenericToken firstToken;
protected GenericToken lastToken;
private GenericToken firstToken;
private GenericToken lastToken;
Copy link
Member

Choose a reason for hiding this comment

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

why do we change the visibility of these? That's a breaking API change

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just because these fields weren't being used in the project, but I haven't considered that perhaps someone with a custom rule is using any of these fields. I'll revert this.

newChild.jjtSetParent(this);
// Finally, report the insert event
// insertChildEvent(this, newChild, insertionIndex); // TODO [autofix]
return insertionIndex;
Copy link
Member

Choose a reason for hiding this comment

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

why are we duplicating all logic currently present in jjtAddChild on this same class?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In fact, I think that we are duplicating only two lines of that method, which are:

children[index] = child;
child.jjtSetChildIndex(index);

When calling jjtAddChild, the given child is inserted at the given index, without shifting any possible existing elements to the right (i.e., if there is a child at that index, it will perform an incomplete replacement - incomplete because it won't correctly unlink the old node being replaced).
However, when calling insert, the inner call to makeSpaceForNewChild ensures not only that the children array has enough space to insert the given new child at the given index, but also that all children from that index on are shifted to the right if there is an existing child at that index. This method also updates all children indexes if a shift is indeed performed.

Note that the newChild.jjtSetParent(this); statement in the insert method is not performed in the jjtAddChild method (which, in my opinion, should be done to fully add/link a child to its parent).

With all this, I consider that currently the logic is not being duplicated.

But please, let me know if you see a way to merge these two methods (jjtAddChild and insert), or if you see with good eyes that jjtAddChild just calls the insert method.

Copy link
Member

Choose a reason for hiding this comment

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

True... still I think it will be very confusing on when to use which method... We need to be far more consistent on our APIs.

}

@Override
public void removeChildAtIndex(final int childIndex) {
Copy link
Member

Choose a reason for hiding this comment

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

removing this method is a breaking API change. We should readd it, have it @Deprecated and internally call to the new remove method if we want to rename it

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the advice. I'll apply this change too.

}

@Override
public void remove(final int index) {
Copy link
Member

Choose a reason for hiding this comment

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

the semantics of this is weird... remove() removes a node from it's parent, but remove(int) removes a child from self?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes. I think it would be nice to just call node.remove().
However, as the other rewrite methods (i.e. insert & replace) are called from the parent node to perform an operation over a child node at a given index, I would choose to stay with remove(int) instead of with remove().

Copy link
Member

@jsotuyod jsotuyod Jan 28, 2018

Choose a reason for hiding this comment

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

The problem is, node.remove() already exists. Actually, you added it yourselves. So we end up with:

  • node.remove() removes self from parent
  • node.remove(int) removes child from self

This lends itself way too much for confusion. We are overloading a method, but in doing so changing completely the semantics! Tidy up the API and be consistent. Same name methods should do the same, regardless of the parameters they receive (how it's invoked).

I'm not particularly happy however at removing a method we just added in 6.0.0. I fear some lack of proper planning on how the feature will work, by focusing too early on the abstract operations on the node (bottom-up), before actually figuring out what the higher layers of abstraction would require (top-down).

We can survive such deprecation if it's the right thing to do for PMD, but I'd really like to be certain we won't keep deprecating newly added methods as we move forward.

@MatiasComercio
Copy link
Contributor Author

@jsotuyod I'll wait for your reply to the remove(int) & insert method comments so as to implement all changes together

@jsotuyod jsotuyod added the is:WIP For PRs that are not fully ready, or issues that are actively being tackled label Jan 28, 2018
@jsotuyod jsotuyod removed this from the 6.1.0 milestone Feb 18, 2018
@MatiasComercio
Copy link
Contributor Author

We are closing this as it has been deprecated in favor of a new autofixes approach. See #693.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
in:autofixes Affects the autofixes framework is:WIP For PRs that are not fully ready, or issues that are actively being tackled
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants