Added -e flag to exclude repair of a file containing DO NOT ALTER OR REMOVE COPYRIGHT NOTICES #10

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 <[email protected]>

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

src/main/java/org/glassfish/copyright/AbstractCopyright.java

@@ -93,6 +95,8 @@
    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

src/main/java/org/glassfish/copyright/AbstractCopyright.java

-	    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

src/main/java/org/glassfish/copyright/AbstractCopyright.java

-	    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

src/main/java/org/glassfish/copyright/AbstractCopyright.java

@@ -332,7 +345,24 @@ else if (lc == null)
 	    System.out.println("No errors: " + file);
    }

-    /**
+	/**


Indentation problem again?

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

src/main/java/org/glassfish/copyright/AbstractCopyright.java

@@ -332,7 +345,24 @@ else if (lc == null)
 	    System.out.println("No errors: " + file);
    }

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


nit typo: "if the file.."

fixed in 620fdb2

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

src/main/java/org/glassfish/copyright/Copyright.java

@@ -39,6 +39,7 @@
 *	-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

src/main/java/org/glassfish/copyright/AbstractCopyright.java

@@ -93,6 +95,8 @@
    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.
+	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 <[email protected]>

…or editing Signed-off-by: CesarHernandezGt <[email protected]>

Signed-off-by: CesarHernandezGt <[email protected]>

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 <[email protected]>

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 <[email protected]>

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

923b4e0

…or editing Signed-off-by: CesarHernandezGt <[email protected]>

Fixed identation for this PR code

39e6c63

Signed-off-by: CesarHernandezGt <[email protected]>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added -e flag to exclude repair of a file containing DO NOT ALTER OR REMOVE COPYRIGHT NOTICES #10

Added -e flag to exclude repair of a file containing DO NOT ALTER OR REMOVE COPYRIGHT NOTICES #10

cesarhernandezgt commented May 2, 2020 •

edited

Loading

scottmarlow commented May 2, 2020

kwsutter left a comment

kwsutter May 4, 2020

cesarhernandezgt May 8, 2020

kwsutter May 4, 2020

kwsutter May 4, 2020

kwsutter May 4, 2020

kwsutter May 4, 2020

cesarhernandezgt May 8, 2020

kwsutter May 4, 2020

kwsutter May 4, 2020

cesarhernandezgt May 8, 2020

kwsutter commented May 4, 2020

romain-grecourt commented May 4, 2020

arjantijms commented May 4, 2020

romain-grecourt commented May 4, 2020

cesarhernandezgt commented May 4, 2020

cesarhernandezgt commented May 8, 2020 •

edited

Loading

bshannon commented May 13, 2020

Added -e flag to exclude repair of a file containing DO NOT ALTER OR REMOVE COPYRIGHT NOTICES #10

Are you sure you want to change the base?

Added -e flag to exclude repair of a file containing DO NOT ALTER OR REMOVE COPYRIGHT NOTICES #10

Conversation

cesarhernandezgt commented May 2, 2020 • edited Loading

scottmarlow commented May 2, 2020

kwsutter left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kwsutter commented May 4, 2020

romain-grecourt commented May 4, 2020

arjantijms commented May 4, 2020

romain-grecourt commented May 4, 2020

cesarhernandezgt commented May 4, 2020

cesarhernandezgt commented May 8, 2020 • edited Loading

bshannon commented May 13, 2020

cesarhernandezgt commented May 2, 2020 •

edited

Loading

cesarhernandezgt commented May 8, 2020 •

edited

Loading