Code cleanup

[eclipse-birt]

Code cleanup

[Flugtiger]

See below Wim Jongman <[email protected]> schrieb am Mo., 8. März 2021, 22:10:

@Flugtiger <https://github.com/Flugtiger> @ruspl-afed <https://github.com/ruspl-afed> Let's discuss here: So we first settle on Java 8?

+1

Do we want to do code cleanup for all stuff available in java 8: - Convert to lambda, - remove whitespace, - move to enhanced for loops, - etc.. I'm in for everything that can be done automatically (e.g. code

formatting) but I wouldn't want that someone has to go through the whole codebase and e.g. replace loops or anonymous classes with lambdas. These are things that should be done for new code and replaced in old code when it is touched for other reasons.

…

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#600 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAYA6OBGZ2TMHPGOPLDRYPTTCU4KNANCNFSM4YX43TKQ> .

[wimjongman]

@Flugtiger @ruspl-afed Let's discuss here:

So we first settle on Java 8?

Do we want to do code cleanup for all stuff available in java 8:

• Convert to lambda,
• remove whitespace,
• move to enhanced for loops,
• etc..

[wimjongman]

formatting) but I wouldn't want that someone has to go through the whole
codebase and e.g. replace loops or anonymous classes with lambdas. These
are things that should be done for new code and replaced in old code when
it is touched for other reasons.

Replacing old loops and anonymous inner classes will increase readability.

There is automatic code clean-up for that. If we do reformatting, then all lines will change anyway.

About formatting: BIRT Core is using this code style in many, but not all, places. Note the unusual placement of the brackets and the spaces between parentheses.

   public static String getFolderPath( final String rootPath,
            final String entryName )
    {
        if ( path.charAt( path.length( ) - 1 ) != '/' )
        {
            path = path + '/';
        }
        return path;
    }

[ruspl-afed]

@wimjongman do you plan to use the code style from Eclipse Platform?

Do you think it is reasonable to have a template setting files stored separately that one can use for any new (and existing) bundle?

[wimjongman]

do you plan to use the code style from Eclipse Platform?

That is a logical choice.

Do you think it is reasonable to have a template setting files stored separately

We should have a template, but that template is copied into every project (.settings). This prevents ppl from accidentally formatting without the template. I made a cleanup template here.

We can also skip the cleanup for now and worry about that later. Only move to JDK 8. Maybe I am biting off too much for this moment.

[pipebaum]

I like the idea of just take on JDK 8 for now.
In terms of Format, how about we make a recommendation that anyone that would like to make a pull request should:
a) Implement the template in the .settings for the project
b) Format the code and create a stand-alone pull request
c) Submit a separate pull request with their change using the new format.
Ideally, people would format more than their own file(s), but it is a lot of work and I don't want to get in the way of progress.
Thoughts?

[wimjongman]

I'm working on a version-independent build. Moving to a recent LTS should be a priority after 4.9

the a) b) c) workflow is a good idea, but it will take a lot of extra time and checking. Eclipse can reformat/clean the entire source code in a couple of minutes. I recommend reformating/cleaning code in one step and be done with it.

Also, I like the suggestion of using the code formatting rules that Platform is using. It saves us time to figure out how to do it.

[pipebaum]

+1 to just letting Eclipse do it and be done with it.
+1 to using default Format rules.

I think this is one of those bite the bullet and get it done things.
So who is feeling brave(ish)?

[SteveSchafer-Innovent]

I can take this on, but first I need to know how to correctly import the code into eclipse. Right now I have 1125 errors.

[wimjongman]

Cool, you will find a project called "target platform". In there is the target platform Alex created. Just open it with the TP editor and set as target platform. That should kill most of the issues.

[SteveSchafer-Innovent]

The file appears to be /build/org.eclipse.birt.target/org.eclipse.birt.target.target which can be opened with the target editor. In the upper right corner is "use as active platform" or something like that. After clicking on that I now have 1104 errors. The vast majority of them are similar to this:

Plugin execution not covered by lifecycle configuration: org.apache.maven.plugins:maven-antrun-plugin:1.8:run (execution: generate.property.files, phase: compile) pom.xml

There are a lot of lifecycle problems for various maven plugins.

Maven build from the command line works fine.

[SteveSchafer-Innovent]

I can do this even if the projects have errors in my workspace, although it would be nice to get rid of them. I think I'll open a separate issue for that. For formatting I suggest two phases: Phase 1: Update all the .settings files (org.eclipse.jdt.core.prefs and org.eclipse.jdt.ui.prefs) to implement the formatting rules. Phase 2: Reformat all the code. Phase 2 especially needs to be a separate commit from any functional changes. Both phases can be part of the same pull request or they can be separate. Let me know if you have a preference.

[pipebaum]

Steve, I think the plan is to just use the default format settings instead of updating all of the .settings files. Please check with Wim.

…

On Tue, Mar 30, 2021 at 5:12 PM Steve Schafer ***@***.***> wrote: I can do this even if the projects have errors in my workspace, although it would be nice to get rid of them. I think I'll open a separate issue for that. For formatting I suggest two phases: Phase 1: Update all the .settings files (org.eclipse.jdt.core.prefs and org.eclipse.jdt.ui.prefs) to implement the formatting rules. Phase 2: Reformat all the code. Phase 2 especially needs to be a separate commit from any functional changes. Both phases can be part of the same pull request or they can be separate. Let me know if you have a preference. — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#600 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ABKF2BMEKJ2OLOMBK2Y6DWDTGJEGFANCNFSM4YX43TKQ> .

-- Scott Rosenbaum Innovent Solutions, Inc. 612 220 6006 cell

[SteveSchafer-Innovent]

Just experimentally, if I use the default formatting at the project level, it puts a bunch of property settings in the settings file. I think we need to enable formatting at the project level, otherwise we have no control over what people have set in the workspace. Of course we could just ask them nicely...

[ruspl-afed]

+1 for putting .settings to each project

For maven problems please try Quick Fix, the good part of lifecycle configuration porblems should be resolved by installing m2e handlers. For the rest we could consider adding ignorance to the corresponding pom.xml

[wimjongman]

Yes, the .settings need to be there for every project and for every project the same formatting, code cleanup and save actions rules which we get from the Platform project (e.g. o.e.core.runtime).

e.g. https://github.com/eclipse/eclipse.platform.ui/tree/master/bundles/org.eclipse.ui/.settings

[SteveSchafer-Innovent]

I'll update all the .settings files. Let me know if there's any reason I shouldn't copy the files from eclipse.platform.ui verbatim.

[SteveSchafer-Innovent]

Questions:

1. Should we provide .settings if the project does not have a java nature? (e.g. parent projects which only have a maven nature)
2. There are a few differences between existing setting files and those in Platform, mostly the compiler compliance and source versions and some others. Should we only add new properties from Platform and leave all existing properties as-is?

[wimjongman]

1. If there is no java then probably not
2. Keep the existing values if it makes more sense, like the ones you mention.

[SteveSchafer-Innovent]

@wimjongman I sent in a pull request.  This commit only includes the additions and changes to the .settings files.  There are more errors in eclipse now because of some of the error settings, e.g. non-externalized strings is now an error in some projects.  If you think I should fix the code before merging I can do that. It may be a much bigger project. On the other hand we can relax the error settings. Your call.

[wimjongman]

Relax the settings for now but be sure to document them. We want to activate them later.

[SteveSchafer-Innovent]

I've created a new pull request that relaxes the error settings as much as possible. I think it's now time to create a single commit that re-formats all the code. Agreed?

[wimjongman]

+1 from me

[pipebaum]

+1 Fire away...

…

On Tue, Apr 6, 2021 at 9:32 AM Wim Jongman ***@***.***> wrote: +1 from me — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#600 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ABKF2BKAX47WTMNU34IOFDTTHMLQLANCNFSM4YX43TKQ> .

-- Scott Rosenbaum Innovent Solutions, Inc. 612 220 6006 cell

[SteveSchafer-Innovent]

I created a second pull request with all code reformatted. Github couldn't automatically merge.

[xavierdpt]

Hello all,

I was personally frustrated with mvn clean package or mvn clean install failing with the current code base, and it is necessary to do mvn install first. I think this may put off some devs as it makes the project appear unstable.

I found that it is because the clean phase invokes maven-antrun-plugin, but this plugin requires the resolution of all the dependencies, while the maven-clean-plugin does not require the resolution of any dependencies. Additionally, generated files should be placed in /target, which will be automatically removed by the maven-clean-plugin, and the generated files are currently not in /target but caught by .gitignore, so these executions of the maven-antrun-plugin are not necessary.

I can make a PR for this, please tell me if you are interested, but the hardest part was understanding why mvn clean was failing on an empty project. Once this is understood, the steps are the following:

• [required] Grep for clean and remove any invocation of maven-antrun-plugin that comes up
• [optional] Remove all clean targets in build.xml, which are now dead code
• [optional] Update the paths of generated files to go in /target and cleanup .gitignore

With these changes, I have a working mvn clean package dev cycle that does not require the installation of the snapshot version in the local repository.

I am asking because maybe you want to finish some other tasks before accepting contributions, if so, you can integrate these changes in a PR of your own.

Cheers.

[wimjongman]

Hi Xavier, yes, please make a PR so we can take a look at it.

[xavierdpt]

It is easy to remove the calls to the maven antrun plugin, and to also delete the clean target from the ant scripts which become irrelevant.
But it's more difficult to update all the paths so that all the generated files fall in /target, because some of these files are also used by Eclipse and I have no idea what the consequences would be and my undersranding of Eclipse is too limited.