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

RFD CommentsRefactoring - any plans for adding parameters? #351

Open
cal101 opened this issue May 25, 2018 · 3 comments
Open

RFD CommentsRefactoring - any plans for adding parameters? #351

cal101 opened this issue May 25, 2018 · 3 comments

Comments

@cal101
Copy link
Collaborator

cal101 commented May 25, 2018

Hi again!

I was surprised about the good results of the comments refactoring.
There is one kind of change I would like to suppress:

If one or a bunch of constants ("public|private final ...") is commented out with line comments I don't want them to be converted to java doc comments. Could be easily matched by some regular expression.

Are there any plans for adding configuration parameters to AutoRefactor? Or do you consider that a philosophical standpoint that it should not be possible to do that?
Would you consider configuration parameters on code level?

Have fun,
cal

@JnRouvignac
Copy link
Owner

JnRouvignac commented Jun 29, 2018

Hey cal101,

I spent a lot of time on the comments refactoring and it was not easy :)

If one or a bunch of constants ("public|private final ...") is commented out with line comments I don't want them to be converted to java doc comments. Could be easily matched by some regular expression.

Generally speaking, the idea is that you should not have code that is commented out in your code.
If you have some, then I suggest you delete them since:

  1. this is an unnecessary distraction for developers / it blurs the existing code
  2. it goes out of synch with the code
  3. this is more than dead code: this is zombie code!

Unless you have good examples, this will probably never happen.

Are there any plans for adding configuration parameters to AutoRefactor? Or do you consider that a philosophical standpoint that it should not be possible to do that?
Would you consider configuration parameters on code level?

What kind of configuration parameters are you looking for?
I prefer to avoid them because testing becomes a nightmare.
I prefer rules that work out of the box. This has always been the philosophy of this tool (although controversial).

Having fun here. How about you? :)

@cal101
Copy link
Collaborator Author

cal101 commented Jul 2, 2018

Moin Jean-Noël,

I re-read my initial post now and I should have written a better introduction. I tend to
write not enough of the motivations and pre-thoughts I had.
The comment refactoring is a very good work especially when knowing how "easy" dealing with comments in jdt ast is.
Sorry, I didn't want to talk the work small. In the opposite. :-(
I didn't gave comment refactoring enough attention before and running the refactoring it more just for fun changed my mind when I saw the result.

Now for the details :-)

I applied it to a code base with about 20 years of history, about 16k of files.
89% of the formatting results are good, 10% are acceptable and less than 1% are ugly, wrong or otherwise not acceptable.
And often it's only a small portion of the file that follows a well known style in the code base that get's in the way.
For various reasons I can't or don't want to change the code here but still want to improve comment formatting of the larger parts of the files.
Since the AutoRefactor tool is a 99% fit i try to fix or improve it instead of doing something else.

I started writing down examples of code I don't want to autorefactor but soon realized that
except one case (that I consider a bug) it boils down to:
The priority of uniformity of code, normalized code and best practices of today is overruled by other rules.
And the decision is not done based on lazyness. ;-)

Let's get the bug out first:

... some long method
} // once upon a time people had the habit of adding some more or less useful comment about the preceding method here

public void innocentMethodThatGetsUnrelatedPublicComment()

Such comments won't get through code review today but they exist and are clearly located at the previous method and should not become javadoc of the following.

I am having fun with stuff like stat at the moment. (Really ;-)
I wrote about that on gitter and wanted to discuss that.
I am doing ci server pipeline based batch refactorings/transformations on a legacy code base.
When getting the pipelines up i added autorefactor steps as test cases because
that was easy using autorefactor cli.
They may become part of the officially and reviewed part but if and what is open.
So I am talking about repeated automatic batch application on bulk legacy code here.
Not the typical run of AutoRefactor on a few classes in the IDE.

I will add autorefactor steps to my groovy based transformation dsl.
That makes compilation unit based includes/excludes a nobrainer.

But for some autorefactor rules configurations would be handy:

TransformationDsl.script {
    build()
    rules {
        // apply to whole workspace
        inputWorkspace()

        // the compilation units of these top level classes are excluded
        excludeClass '**.some.pkg.generated.**'
        excludeClass '**.*DoNotTouchMe'

        editAst {
            // some ast rewrite removing a level of ()
            replaceMatchWithMarked parenthesizedExpression().hasExpression(
                parenthesizedExpression().hasExpression(expression().bind("replacement"))))
        }

        autorefactor.comments {
            ignoreComment ~'((TODO)|(CONFIDENTIAL))'
            ignoreComment ~'^\s*((private)|(public))'
        }
        autorefactor.useUnqualifiedNames {
            // an imaginary "InnerClass" not listed,
            // so pkg.A gets replaced with A,
            // but pkg.A.B gets replacec with A.B, not B
            use TopLevelClass, TopLevelMethod
            excludeMethod 'of’, 'copyOf'
            excludeMethod ~'.*create[0-9]*'
        }
    }
}

I understand that configuration parameters can make the test input explode.
The parameters i currently imagine are opt-in/opt-out style.
Indeed I think all i have thought of yet were opt-out ;-)
You can opt-in into autorefactor by selecting the rules to apply
but on rule level we are up to your mercy ;-)

If I find some time I will try to make up a real example that can be discussed.

Bye,
Cal

@JnRouvignac
Copy link
Owner

Don't worry I did not think at all you were trying to diminish the work on the comments refactoring. Because I spent quite a lot of time on it, so I am very happy you like it.

Sorry for missing the discussion on gitter, my availability has reduced drastically since I started this project.

Your ideas for configuring comments refactoring (in the DSL) are interesting. Please raise issues for these. For the "code" bug you reported, I am sorry, we can't do much about it.

For others, it is probably best to do like you generally do: open specific bugs/RFEs for each rule. Some problems may be directly fixed inside the rule itself. Others may require some kind of configuration indeed. We'll have to figure out a way to test it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants