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

Use visitor for applying changes with locations #171

Merged
merged 15 commits into from
Apr 20, 2023

Conversation

nimakarimipour
Copy link
Member

@nimakarimipour nimakarimipour commented Apr 4, 2023

This PR updates the logic for applying changes specified by Location instances using the visitor pattern architecture added in #169

@nimakarimipour nimakarimipour added the refactoring/simplification Refactoring Simplification label Apr 4, 2023
@nimakarimipour nimakarimipour self-assigned this Apr 4, 2023
@nimakarimipour nimakarimipour changed the base branch from master to nimak/add-visitor-loc April 4, 2023 23:18
* @return the modification that should be applied.
*/
@Nullable
public Modification visit(Change change) {
Copy link
Member

Choose a reason for hiding this comment

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

It is confusing to name this method visit since it's not one of the standard visitor methods. How about:

Suggested change
public Modification visit(Change change) {
public Modification computeModification(Change change) {

Copy link
Member Author

Choose a reason for hiding this comment

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

Sure, that makes sense. 15ff010

}

/**
* Applies the change to the compilation unit.
Copy link
Member

Choose a reason for hiding this comment

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

This doesn't actually apply any change, right? It just computes the change (Modification) to be applied?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes exactly. I reworded the java doc to mention this, please let me know if I should update it. 2e2dd6b

Copy link
Member

Choose a reason for hiding this comment

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

Hrm, I don't see the javadoc changes here; maybe the commit got lost somehow?

Copy link
Member Author

Choose a reason for hiding this comment

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

Sorry for that, the change was on the javadoc for the class. Update the method javadoc as well 66edb7e

nimakarimipour added a commit that referenced this pull request Apr 19, 2023
This PR adds the appropriate architecture and API to apply a visitor for `Location` instances. This is preparation for the upcoming refactoring. #170 and #171
Base automatically changed from nimak/add-visitor-loc to master April 19, 2023 19:58
@nimakarimipour nimakarimipour requested a review from msridhar April 19, 2023 21:07
Copy link
Member

@msridhar msridhar left a comment

Choose a reason for hiding this comment

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

one minor thing

}

/**
* Applies the change to the compilation unit.
Copy link
Member

Choose a reason for hiding this comment

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

Hrm, I don't see the javadoc changes here; maybe the commit got lost somehow?

@nimakarimipour nimakarimipour requested a review from msridhar April 20, 2023 21:00
@nimakarimipour nimakarimipour merged commit 10102ff into master Apr 20, 2023
@nimakarimipour nimakarimipour deleted the nimak/visitor-for-change branch April 20, 2023 21:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
refactoring/simplification Refactoring Simplification
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants