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] Rule Violation Fixes applier flow for AST #856

Closed
wants to merge 4 commits into from

Conversation

gibarsin
Copy link
Contributor

Summary

Implements #693:

  • Add a way to store autofixes for proper call
  • Apply stored autofixes over AST nodes

RuleViolationFix operations for AST should be stored and applied after application of rule chain and application of every rule so that the following rule may have the most updated representation of the AST.

Additions

  • Add RuleViolationFixContext which holds the the class of an implementation of RuleViolationFix and the node on which to operate.
  • Add RuleViolationFixesApplier which stores, in order of arrival, instances of RuleViolationFixContext. The operations may then be applied only once. The instance is reusable if it is cleared first.

Note

Consider looking only at the following commit which is dependant on the commits of #850: e9a3070. After merging the mentioned PR, this note can be deleted.

@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
Copy link
Member

@jsotuyod jsotuyod left a comment

Choose a reason for hiding this comment

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

This PR has a lot of repeated code from #850 which makes it really hard to audit. Please, rework it to have it separately.

* @param applicableCompilationUnits the list of applicable compilation units on which to apply and fix the rules
* @param context the context which the visitors will have
*/
public void applyWithAutoFixes(final List<? extends Node> applicableCompilationUnits, final RuleContext context) {
Copy link
Member

Choose a reason for hiding this comment

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

do we really need to copy / paste 30+ lines to have an exact duplicate of apply that merely adds a context.getRuleViolationFixesApplier().applyAutoFixesAndClear();?

* @param context the context passed to the visitors
* @param language The AST source
*/
public void applyWithAutoFixes(final List<Node> applicableCompilationUnits, final RuleContext context, final Language language) {
Copy link
Member

Choose a reason for hiding this comment

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

ditto

/**
* Apply, in order of insertion, the auto fixes for the nodes. In the end it calls the {@link #clear()} method.
*/
public void applyAutoFixesAndClear() {
Copy link
Member

Choose a reason for hiding this comment

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

what's the benefit in reusing the same instance? Why do we keep both versions of these methods? It seems to me we will always want to use the later, and that it's a hack just to avoid instantiating more RuleViolationFixesApplier. To what end, I'm not sure

@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
@gibarsin
Copy link
Contributor Author

gibarsin commented Apr 8, 2018

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

@gibarsin gibarsin closed this Apr 8, 2018
@gibarsin gibarsin deleted the ruleViolationFixesApplier branch April 27, 2018 15:28
@gibarsin gibarsin restored the ruleViolationFixesApplier branch April 27, 2018 15:28
@gibarsin gibarsin deleted the ruleViolationFixesApplier branch April 27, 2018 15:29
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