Added -e flag to exclude repair of a file containing DO NOT ALTER OR REMOVE COPYRIGHT NOTICES by cesarhernandezgt · Pull Request #10 · eclipse-ee4j/glassfish-copyright-plugin

cesarhernandezgt · 2020-05-02T02:21:30Z

Fixes #9
How to test this PR:

mvn clean install
Assuming you are using command line execution with maven installed in ~

$ java -cp  ~/.m2/repository/org/glassfish/copyright/glassfish-copyright-maven-plugin/2.4-SNAPSHOT/glassfish-copyright-maven-plugin-2.4-SNAPSHOT.jar org.glassfish.copyright.Copyright -c -r -e /Users/cesar/git/jakartaee-tck/lib/schemas/connector_1_5.xsd

The output would be:

/Users/cesar/git/jakartaee-tck/lib/schemas/connector_1_5.xsd: WARNING: contains:       DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS HEADER.
/Users/cesar/git/jakartaee-tck/lib/schemas/connector_1_5.xsd: WARNING: extra copyright:       Copyright 2003-2007 Sun Microsystems, Inc. All rights reserved.
/Users/cesar/git/jakartaee-tck/lib/schemas/connector_1_5.xsd:  SKIPPED: it contains: DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS HEADER.

The repair is not applied if the file contains the Do not alter or remove notices.

…REMOVE COPYRIGHT NOTICES Signed-off-by: CesarHernandezGt <chernandez@tomitribe.com>

scottmarlow · 2020-05-02T13:27:32Z

I noticed that this project uses tab instead of spaces, not sure if that is an issue for this pr.

kwsutter

I think the main issue is the indentation. I didn't mark every occurrence via this review, but I think you'll get the picture. Thanks.

kwsutter · 2020-05-04T14:11:03Z

    private static Pattern bsdpat = Pattern.compile(
 	"(THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS)"+
 	"|(SPDX-License-Identifier: BSD-3-Clause)", Pattern.MULTILINE);
+	// patter to detect DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS HEADER.


nit typo: "pattern"

fixed in 620fdb2

kwsutter · 2020-05-04T14:12:06Z

-	    if (c.warn && !c.quiet)
-		warnCopyright(file, r);
+	    if (c.warn && !c.quiet){
+			warnCopyright(file, r);


Formatting doesn't look right. Are you using tabs or spaces (preferred)?

kwsutter · 2020-05-04T14:12:23Z

-	    if (c.warn && !c.quiet)
-		warnCopyright(file, r);
+	    if (c.warn && !c.quiet){
+			warnCopyright(file, r);


Formatting doesn't look right. Are you using tabs or spaces (preferred)? Way too much indentation.

kwsutter · 2020-05-04T14:13:11Z

    }

-    /**
+	/**


Indentation problem again?

kwsutter · 2020-05-04T14:13:28Z


-    /**
+	/**
+	 * Verifies if he file contains the message:


nit typo: "if the file.."

fixed in 620fdb2

kwsutter · 2020-05-04T14:14:13Z

 *	-x	check XML syntax files
 *	-p	check properties syntax files
 *	-t	check other text files
+ *  -e  exclude files that contains DO NOT ALTER OR REMOVE COPYRIGHT NOTICES


indentation again...

kwsutter · 2020-05-04T14:17:52Z

 	"(THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS)"+
 	"|(SPDX-License-Identifier: BSD-3-Clause)", Pattern.MULTILINE);
+	// patter to detect DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS HEADER.
+	protected static Pattern dontpat = Pattern.compile(DONT_ALTER_HEADER);


Personally, I'd make this variable name a little easier to understand and readable. I know you were just following the pattern for the "bsdpat" variable, but I would think something like "doNotAlterPattern" would be clearer. I'll leave it to you though.

fixed in 620fdb2

kwsutter · 2020-05-04T14:20:15Z

I noticed that this project uses tab instead of spaces, not sure if that is an issue for this pr.

I agree, Scott. Spaces should be used instead of tabs.

romain-grecourt · 2020-05-04T18:56:51Z

IIRC all the sources in this project are formatted with tabs. CC @bshannon

arjantijms · 2020-05-04T20:16:05Z

IIRC all the sources in this project are formatted with tabs. CC @bshannon

According to the Jakarta Code Convention it should indeed be spaces. Although we had it in words once (it was lost during a wiki transfer, and should be redone), it's in code here:

https://github.com/eclipse-ee4j/ee4j/tree/master/codestyle

romain-grecourt · 2020-05-04T20:30:30Z

I'm not disagreeing with the rules :) This project was donated as-is and not converted to spaces. @bshannon is the original author, I just wanted to make sure he knows about this discussion.

cesarhernandezgt · 2020-05-04T23:24:56Z

Thank you for the review so far. I'll polish the variable name and fix the typos during the week.

About the space vs tab, I don't have any strong opinion. My proposal would be:

Convert the current PR spaces to tabs in order to comply with the current project code format.
If we all agree that the project will follow the ee4j codestyle, I would vote to do this on a separate PR in order to keep the scope of current PR focused on the new flag.

Signed-off-by: CesarHernandezGt <chernandez@tomitribe.com>

…or editing Signed-off-by: CesarHernandezGt <chernandez@tomitribe.com>

Signed-off-by: CesarHernandezGt <chernandez@tomitribe.com>

cesarhernandezgt · 2020-05-08T22:08:16Z

Thanks for the review so far.
In 620fdb2 I ended up importing https://github.com/eclipse-ee4j/ee4j/blob/master/codestyle/ee4j_intellij_formatting.xml and then applying a cmd+l to allow the IDE to auto-format the code.

As you can check that didn't resolve the majority of the indentation issues.
Then in 923b4e0 and 39e6c63 I disable in my IDE the "Detect and use existing file indents for editing" option and give a try again using the imported code format.
Hope this time it's better.

bshannon · 2020-05-13T02:44:49Z

IIRC all the sources in this project are formatted with tabs. CC @bshannon

Rule # 0 - follow the existing project conventions.
Rule #1 - use new project conventions for substantial new code.

Bottom line, please don't change it.

Added -e flag to exclude repair of a file containing DO NOT ALTER OR …

93cc228

…REMOVE COPYRIGHT NOTICES Signed-off-by: CesarHernandezGt <chernandez@tomitribe.com>

kwsutter suggested changes May 4, 2020

View reviewed changes

kwsutter mentioned this pull request May 4, 2020

Do not alter Copyright notice in files that contain "DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS HEADER" #9

Open

cesarhernandezgt added 3 commits May 8, 2020 15:35

Applied to this PR the ee4j codeStyle

620fdb2

Signed-off-by: CesarHernandezGt <chernandez@tomitribe.com>

Updated PR code format without detect and use existing file indents f…

923b4e0

…or editing Signed-off-by: CesarHernandezGt <chernandez@tomitribe.com>

Fixed identation for this PR code

39e6c63

Signed-off-by: CesarHernandezGt <chernandez@tomitribe.com>

                   }
-                  /**
+              	/**

Conversation

cesarhernandezgt commented May 2, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

scottmarlow commented May 2, 2020

Uh oh!

kwsutter left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kwsutter commented May 4, 2020

Uh oh!

romain-grecourt commented May 4, 2020

Uh oh!

arjantijms commented May 4, 2020

Uh oh!

romain-grecourt commented May 4, 2020

Uh oh!

cesarhernandezgt commented May 4, 2020

Uh oh!

cesarhernandezgt commented May 8, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bshannon commented May 13, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

cesarhernandezgt commented May 2, 2020 •

edited

Loading

cesarhernandezgt commented May 8, 2020 •

edited

Loading