matrei commented on code in PR #14903: URL: https://github.com/apache/grails-core/pull/14903#discussion_r2212573311
########## gradle/style-enforcement-config.gradle: ########## @@ -0,0 +1,73 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * https://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +apply plugin: 'checkstyle' +apply plugin: 'com.diffplug.spotless' +apply plugin: 'io.spring.nohttp' // enforce https everywhere + +spotless { + java { + licenseHeaderFile(rootProject.file("etc/config/spotless/license.java")) + target('src/main/java/**/*.java', 'src/main/groovy/**.java', 'grails-app/**.java') + } + groovy { + licenseHeaderFile(rootProject.file("etc/config/spotless/license.java")) + target('src/main/groovy/**.groovy', 'grails-app/**.groovy') + } + format 'javaMisc', { + target('src/main/**/package-info.java', 'src/main/**/module-info.java') + licenseHeaderFile(rootProject.file('etc/config/spotless/license.java'), '\\/\\*\\*') + } +} + +checkstyle { + configFile = project.rootProject.layout.projectDirectory.file('etc/config/checkstyle/checkstyle.xml').asFile + configDirectory = project.rootProject.layout.projectDirectory.dir('etc/config/checkstyle').asFile + + // Per submodule + maxErrors = 1 + maxWarnings = 10 + + showViolations = true +} + +nohttp { + // TODO: xsd, xml, & doc files need looked at; this will at least make sure our POMs are correct + source.exclude('src/test/**', 'build/**', + 'LICENSE', '**/LICENSE', 'NOTICE', '**/NOTICE', 'licenses/**', 'INSTALL', + '**/*.xsd', '**/*.xml', '**/*.dtd', + '**/test/**', + '**/@grails.codegen.projectClassName@GrailsPlugin.groovy', + '**/grails-doc/**' // TODO: the grails doc still has a lot of work to do to remove httpsm mmmmm + ) +} Review Comment: Suggestion: ```groovy extensions.configure(NoHttpExtension) { // Explicit `it` required in extension configuration // TODO: xsd, xml, & doc files need looked at; this will at least make sure our POMs are correct it.source.exclude( 'build/**', 'src/test/**', 'LICENSE', '**/LICENSE', 'NOTICE', '**/NOTICE', 'licenses/**', 'INSTALL', '**/*.xsd', '**/*.xml', '**/*.dtd', '**/test/**', '**/@grails.codegen.projectClassName@GrailsPlugin.groovy', '**/grails-doc/**' // TODO: the grails doc still has a lot of work to do to remove https ) } ``` ########## grails-async/core/src/main/groovy/grails/async/DelegateAsync.groovy: ########## Review Comment: I tried reverting the changes in this file and run `spotlessApply`. The task did not add the changes back. And even though I changed a source file, the `spotlessApply` task was reported as `UP-TO-DATE`. ########## gradle/style-enforcement-config.gradle: ########## @@ -0,0 +1,73 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * https://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +apply plugin: 'checkstyle' +apply plugin: 'com.diffplug.spotless' +apply plugin: 'io.spring.nohttp' // enforce https everywhere + +spotless { + java { + licenseHeaderFile(rootProject.file("etc/config/spotless/license.java")) + target('src/main/java/**/*.java', 'src/main/groovy/**.java', 'grails-app/**.java') + } + groovy { + licenseHeaderFile(rootProject.file("etc/config/spotless/license.java")) + target('src/main/groovy/**.groovy', 'grails-app/**.groovy') + } + format 'javaMisc', { + target('src/main/**/package-info.java', 'src/main/**/module-info.java') + licenseHeaderFile(rootProject.file('etc/config/spotless/license.java'), '\\/\\*\\*') + } +} + +checkstyle { + configFile = project.rootProject.layout.projectDirectory.file('etc/config/checkstyle/checkstyle.xml').asFile + configDirectory = project.rootProject.layout.projectDirectory.dir('etc/config/checkstyle').asFile + + // Per submodule + maxErrors = 1 + maxWarnings = 10 + + showViolations = true +} + +nohttp { + // TODO: xsd, xml, & doc files need looked at; this will at least make sure our POMs are correct + source.exclude('src/test/**', 'build/**', + 'LICENSE', '**/LICENSE', 'NOTICE', '**/NOTICE', 'licenses/**', 'INSTALL', + '**/*.xsd', '**/*.xml', '**/*.dtd', + '**/test/**', + '**/@grails.codegen.projectClassName@GrailsPlugin.groovy', + '**/grails-doc/**' // TODO: the grails doc still has a lot of work to do to remove httpsm mmmmm + ) +} + +if (tasks.names.contains('checkstyleTest')) { + tasks.named('checkstyleTest').configure { + it.group = 'verification' + it.enabled = false Review Comment: Add comment why we have chosen to disable `checkstyleTest`? ########## grails-async/core/src/main/groovy/grails/async/Promises.groovy: ########## @@ -51,79 +51,79 @@ class Promises { } /** - * @see PromiseFactory#waitAll(grails.async.Promise[]) + * @see PromiseFactory#waitAll(grails.async.Promise [ ]) Review Comment: This does not seem right? More instances below and in other files. ########## grails-async/core/src/main/groovy/org/grails/async/factory/PromiseFactoryBuilder.groovy: ########## @@ -43,11 +41,10 @@ class PromiseFactoryBuilder { List<PromiseFactory> promiseFactories = ServiceLoader.load(PromiseFactory).toList() PromiseFactory promiseFactory - if(promiseFactories.isEmpty()) { + if (promiseFactories.isEmpty()) { log.debug 'No PromiseFactory implementation found. Using default ExecutorService promise factory.' Review Comment: Align on parentheses for method calls? Is there a check for this? ########## grails-async/core/src/main/groovy/grails/async/PromiseFactory.groovy: ########## @@ -50,15 +50,15 @@ interface PromiseFactory { /** * Creates a promise with a value pre-bound to it * @param value The value - * @param <T> The type of the value + * @param <T> The type of the value Review Comment: Hmm... what formatting rule adding this whitespace? ########## gradle/style-enforcement-config.gradle: ########## @@ -0,0 +1,73 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * https://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +apply plugin: 'checkstyle' +apply plugin: 'com.diffplug.spotless' +apply plugin: 'io.spring.nohttp' // enforce https everywhere + +spotless { + java { + licenseHeaderFile(rootProject.file("etc/config/spotless/license.java")) + target('src/main/java/**/*.java', 'src/main/groovy/**.java', 'grails-app/**.java') + } + groovy { + licenseHeaderFile(rootProject.file("etc/config/spotless/license.java")) + target('src/main/groovy/**.groovy', 'grails-app/**.groovy') + } + format 'javaMisc', { + target('src/main/**/package-info.java', 'src/main/**/module-info.java') + licenseHeaderFile(rootProject.file('etc/config/spotless/license.java'), '\\/\\*\\*') + } +} Review Comment: - Use type-safe API? - Align on single quotes for strings? - Align on parentheses for method calls? - Are all targets correct? Suggestion: ```groovy extensions.configure(SpotlessExtension) { // Explicit `it` required in extension configuration it.java { licenseHeaderFile(rootProject.layout.projectDirectory.file('etc/config/spotless/license.java')) target( 'grails-app/**/*.java', 'src/main/groovy/**/*.java', 'src/main/java/**/*.java' ) } it.groovy { licenseHeaderFile(rootProject.layout.projectDirectory.file('etc/config/spotless/license.java')) target( 'grails-app/**/*.groovy', 'src/main/groovy/**/*.groovy' ) } it.format('javaMisc') { licenseHeaderFile(rootProject.layout.projectDirectory.file('etc/config/spotless/license.java'), '\\/\\*\\*') target( 'src/main/**/module-info.java', 'src/main/**/package-info.java' ) } } ``` ########## gradle/style-enforcement-config.gradle: ########## @@ -0,0 +1,73 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * https://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +apply plugin: 'checkstyle' +apply plugin: 'com.diffplug.spotless' +apply plugin: 'io.spring.nohttp' // enforce https everywhere + +spotless { + java { + licenseHeaderFile(rootProject.file("etc/config/spotless/license.java")) + target('src/main/java/**/*.java', 'src/main/groovy/**.java', 'grails-app/**.java') + } + groovy { + licenseHeaderFile(rootProject.file("etc/config/spotless/license.java")) + target('src/main/groovy/**.groovy', 'grails-app/**.groovy') + } + format 'javaMisc', { + target('src/main/**/package-info.java', 'src/main/**/module-info.java') + licenseHeaderFile(rootProject.file('etc/config/spotless/license.java'), '\\/\\*\\*') + } +} + +checkstyle { + configFile = project.rootProject.layout.projectDirectory.file('etc/config/checkstyle/checkstyle.xml').asFile + configDirectory = project.rootProject.layout.projectDirectory.dir('etc/config/checkstyle').asFile + + // Per submodule + maxErrors = 1 + maxWarnings = 10 + + showViolations = true +} Review Comment: Suggestion: ```groovy extensions.configure(CheckstyleExtension) { // Explicit `it` required in extension configuration it.configFile = rootProject.layout.projectDirectory.file('etc/config/checkstyle/checkstyle.xml').asFile it.configDirectory = project.rootProject.layout.projectDirectory.dir('etc/config/checkstyle').asFile it.maxErrors = 1 it.maxWarnings = 10 it.showViolations = true } ``` ########## grails-async/core/src/main/groovy/grails/async/DelegateAsync.groovy: ########## @@ -39,7 +39,7 @@ import org.codehaus.groovy.transform.GroovyASTTransformationClass * If the annotation is applied to a new class: * * <pre><code> - * @DelegateAsync(BookApi) + * @DelegateAsync (BookApi) Review Comment: This seems wrong? ########## grails-async/core/src/main/groovy/org/grails/async/factory/SynchronousPromise.groovy: ########## @@ -58,8 +58,12 @@ class SynchronousPromise<T> implements Promise<T> { T get() throws Throwable { if (!executed) { executed = true - try { value = callable.call() } - catch (e) { value = e } + try { + value = callable.call() + } + catch (e) { Review Comment: Pull up `catch` on the same line as the closing `try` curly? ########## grails-bootstrap/src/main/groovy/grails/build/logging/ConsoleLogger.java: ########## @@ -91,15 +92,15 @@ public interface ConsoleLogger { /** * Use to log an error * - * @param msg The message + * @param msg The message Review Comment: Is this enforced by a rule? ########## grails-bootstrap/src/main/groovy/grails/codegen/model/ModelBuilder.groovy: ########## @@ -114,13 +114,13 @@ trait ModelBuilder { @Override Map<String, Object> asMap() { - [className: className, - fullName: fullName, + [className : className, + fullName : fullName, propertyName: propertyName, - modelName: propertyName, + modelName : propertyName, packageName: packageName, packagePath: packagePath, - simpleName: simpleName, + simpleName : simpleName, lowerCaseName: lowerCaseName] as Map<String, Object> Review Comment: I'm not sure about the white space changes here. ########## grails-bootstrap/src/main/groovy/grails/io/support/SystemOutErrCapturer.groovy: ########## @@ -25,32 +25,33 @@ import groovy.transform.CompileStatic */ @CompileStatic class SystemOutErrCapturer { + ByteArrayOutputStream out ByteArrayOutputStream err SystemStreamsRedirector previousState - + SystemOutErrCapturer capture() { out = new ByteArrayOutputStream() err = new ByteArrayOutputStream() previousState = SystemStreamsRedirector.create(null, new PrintStream(out, true), new PrintStream(err, true)).redirect() this } - + SystemOutErrCapturer redirectToNull() { out = null err = null OutputStream nullStream = new NullOutputStream() previousState = SystemStreamsRedirector.create(null, new PrintStream(nullStream, true), new PrintStream(nullStream, true)).redirect() this } - + void close() { - if(previousState != null) { + if (previousState != null) { previousState.redirect() previousState = null } } - + public static <T> T withCapturedOutput(Closure<T> closure) { Review Comment: Can we have a rule to not declare `public` which is the default visibility in Groovy? ########## grails-async/core/src/main/groovy/org/grails/async/transform/internal/DelegateAsyncTransactionalMethodTransformer.java: ########## @@ -29,5 +29,6 @@ * @since 2.3 */ public interface DelegateAsyncTransactionalMethodTransformer { + Review Comment: This is good, but I thought you said there was a problem enforcing blank line after type definition? ########## grails-cache/src/test/groovy/grails/plugin/cache/CacheTagLibSpec.groovy: ########## Review Comment: I thought test sources where not touched by this PR? ########## grails-bootstrap/src/main/groovy/grails/build/support/MetaClassRegistryCleaner.java: ########## @@ -32,13 +41,13 @@ * @since 2.0 */ @SuppressWarnings("rawtypes") -public class MetaClassRegistryCleaner implements MetaClassRegistryChangeEventListener { +public final class MetaClassRegistryCleaner implements MetaClassRegistryChangeEventListener { Review Comment: Is this a formatting change? ########## grails-cache/src/main/groovy/grails/plugin/cache/GrailsConcurrentLinkedMapCache.java: ########## @@ -31,143 +31,148 @@ */ public class GrailsConcurrentLinkedMapCache implements GrailsCache { - private static final Object NULL_HOLDER = new NullHolder(); - private String name; - private long capacity; - private final ConcurrentLinkedHashMap<Object, Object> store; - private final boolean allowNullValues; - - /** - * Create a new GrailsConcurrentLinkedMapCache with the specified name - * and capacity - * @param name the name of the cache - * @param capacity of the map - * @param allowNullValues null values (default is true) - */ - public GrailsConcurrentLinkedMapCache(String name, long capacity, boolean allowNullValues) { - this.name = name; - this.capacity = capacity; - this.allowNullValues = allowNullValues; - // Workaround: using explicit type arguments to prevent groovydoc error (#53) - // Replace with diamond operator once a fix for GROOVY-8628 is included in groovy dependency - this.store = new ConcurrentLinkedHashMap.Builder<Object, Object>() - .maximumWeightedCapacity(capacity) - .build(); - } - - /** - * Create a new GrailsConcurrentLinkedMapCache with the specified name - * and capacity - * @param name the name of the cache - * @param capacity of the map - */ - public GrailsConcurrentLinkedMapCache(String name, long capacity) { - this(name, capacity, true); - } - - public final long getCapacity() { - return this.store.capacity(); - } - - @Override - public final String getName() { - return this.name; - } - - @Override - public final ConcurrentMap<Object, Object> getNativeCache() { - return this.store; - } - - public final int getSize() { - return this.store.size(); - } - - public final boolean isAllowNullValues() { - return this.allowNullValues; - } - - @Override - public GrailsValueWrapper get(Object key) { - Object value = getNativeCache().get(key); - return value == null ? null : new GrailsValueWrapper(fromStoreValue(value), null); - } - - @Override - public <T> T get(Object key, Class<T> type) { - Object value = getNativeCache().get(key); - if (value != null && type != null && !type.isInstance(value)) { - throw new IllegalStateException("Cached value is not of required type [" + type.getName() + "]: " + value); - } - return (T) value; - } + private static final Object NULL_HOLDER = new NullHolder(); + private String name; + private long capacity; + private final ConcurrentLinkedHashMap<Object, Object> store; + private final boolean allowNullValues; + + /** + * Create a new GrailsConcurrentLinkedMapCache with the specified name + * and capacity + * + * @param name the name of the cache + * @param capacity of the map + * @param allowNullValues null values (default is true) + */ + public GrailsConcurrentLinkedMapCache(String name, long capacity, boolean allowNullValues) { + this.name = name; + this.capacity = capacity; + this.allowNullValues = allowNullValues; + // Workaround: using explicit type arguments to prevent groovydoc error (#53) + // Replace with diamond operator once a fix for GROOVY-8628 is included in groovy dependency + this.store = new ConcurrentLinkedHashMap.Builder<Object, Object>() + .maximumWeightedCapacity(capacity) + .build(); + } + + /** + * Create a new GrailsConcurrentLinkedMapCache with the specified name + * and capacity + * + * @param name the name of the cache + * @param capacity of the map + */ + public GrailsConcurrentLinkedMapCache(String name, long capacity) { + this(name, capacity, true); + } + + public final long getCapacity() { + return this.store.capacity(); + } + + @Override + public final String getName() { + return this.name; + } + + @Override + public final ConcurrentMap<Object, Object> getNativeCache() { + return this.store; + } + + public final int getSize() { + return this.store.size(); + } + + public final boolean isAllowNullValues() { + return this.allowNullValues; + } + + @Override + public GrailsValueWrapper get(Object key) { + Object value = getNativeCache().get(key); + return value == null ? null : new GrailsValueWrapper(fromStoreValue(value), null); + } + + @Override + public <T> T get(Object key, Class<T> type) { + Object value = getNativeCache().get(key); + if (value != null && type != null && !type.isInstance(value)) { + throw new IllegalStateException("Cached value is not of required type [" + type.getName() + "]: " + value); + } + return (T) value; + } @Override public <T> T get(Object key, Callable<T> valueLoader) { throw new UnsupportedOperationException(); } @SuppressWarnings("unchecked") - public Collection<Object> getAllKeys() { - return getNativeCache().keySet(); - } - - public Collection<Object> getHottestKeys() { - return this.store.descendingKeySet(); - } - - @Override - public void put(Object key, Object value) { - this.store.put(key, toStoreValue(value)); - } - - public ValueWrapper putIfAbsent(Object key, Object value) { - Object existing = this.store.putIfAbsent(key, value); - return toWrapper(existing); - } - - @Override - public void evict(Object key) { - this.store.remove(key); - } - - @Override - public void clear() { - this.store.clear(); - } - - /** - * Convert the given value from the internal store to a user value - * returned from the get method (adapting {@code null}). - * @param storeValue the store value - * @return the value to return to the user - */ - protected Object fromStoreValue(Object storeValue) { - if (this.allowNullValues && storeValue == NULL_HOLDER) { - return null; - } - return storeValue; - } - - /** - * Convert the given user value, as passed into the put method, - * to a value in the internal store (adapting {@code null}). - * @param userValue the given user value - * @return the value to store - */ - protected Object toStoreValue(Object userValue) { - if (this.allowNullValues && userValue == null) { - return NULL_HOLDER; - } - return userValue; - } - - private ValueWrapper toWrapper(Object value) { - return (value != null ? new SimpleValueWrapper(fromStoreValue(value)) : null); - } - - @SuppressWarnings("serial") - - private static class NullHolder implements Serializable { - } + public Collection<Object> getAllKeys() { + return getNativeCache().keySet(); + } + + public Collection<Object> getHottestKeys() { + return this.store.descendingKeySet(); + } + + @Override + public void put(Object key, Object value) { + this.store.put(key, toStoreValue(value)); + } + + public ValueWrapper putIfAbsent(Object key, Object value) { + Object existing = this.store.putIfAbsent(key, value); + return toWrapper(existing); + } + + @Override + public void evict(Object key) { + this.store.remove(key); + } + + @Override + public void clear() { + this.store.clear(); + } + + /** + * Convert the given value from the internal store to a user value + * returned from the get method (adapting {@code null}). + * + * @param storeValue the store value + * @return the value to return to the user + */ + protected Object fromStoreValue(Object storeValue) { + if (this.allowNullValues && storeValue == NULL_HOLDER) { + return null; + } + return storeValue; + } + + /** + * Convert the given user value, as passed into the put method, + * to a value in the internal store (adapting {@code null}). + * + * @param userValue the given user value + * @return the value to store + */ + protected Object toStoreValue(Object userValue) { + if (this.allowNullValues && userValue == null) { + return NULL_HOLDER; + } + return userValue; + } + + private ValueWrapper toWrapper(Object value) { + return (value != null ? new SimpleValueWrapper(fromStoreValue(value)) : null); + } + + @SuppressWarnings("serial") + Review Comment: The blank line here should not be allowed. ########## grails-common/src/main/groovy/org/apache/grails/common/compiler/asm/ClassReader.java: ########## @@ -285,8 +298,7 @@ public String getClassName() { * super class is {@link Object}. * * @return the internal name of super class, or <tt>null</tt> for - * {@link Object} class. - * + * {@link Object} class. Review Comment: Shouldn't this stay indented as it belongs to the `@return` statement on the previous line? ########## grails-common/src/main/groovy/org/apache/grails/common/compiler/asm/ClassReader.java: ########## @@ -298,8 +310,7 @@ public String getSuperName() { * {@link Type#getInternalName() getInternalName}). * * @return the array of internal names for all implemented interfaces or - * <tt>null</tt>. - * + * <tt>null</tt>. Review Comment: Shouldn't this stay indented as it belongs to the `@return` statement on the previous line? ########## grails-common/src/main/groovy/org/apache/grails/common/compiler/asm/ClassReader.java: ########## @@ -803,55 +751,44 @@ private int getAttributes() { /** * Reads an attribute in {@link #b b}. * - * @param attrs - * prototypes of the attributes that must be parsed during the - * visit of the class. Any attribute whose type is not equal to - * the type of one the prototypes is ignored (i.e. an empty - * {@link Attribute} instance is returned). - * @param type - * the type of the attribute. - * @param off - * index of the first byte of the attribute's content in - * {@link #b b}. The 6 attribute header bytes, containing the - * type and the length of the attribute, are not taken into - * account here (they have already been read). - * @param len - * the length of the attribute's content. - * @param buf - * buffer to be used to call {@link #readUTF8 readUTF8}, - * {@link #readClass(int,char[]) readClass} or {@link #readConst - * readConst}. - * @param codeOff - * index of the first byte of code's attribute content in - * {@link #b b}, or -1 if the attribute to be read is not a code - * attribute. The 6 attribute header bytes, containing the type - * and the length of the attribute, are not taken into account - * here. - * @param labels - * the labels of the method's code, or <tt>null</tt> if the - * attribute to be read is not a code attribute. + * @param attrs prototypes of the attributes that must be parsed during the + * visit of the class. Any attribute whose type is not equal to + * the type of one the prototypes is ignored (i.e. an empty + * {@link Attribute} instance is returned). + * @param type the type of the attribute. + * @param off index of the first byte of the attribute's content in + * {@link #b b}. The 6 attribute header bytes, containing the + * type and the length of the attribute, are not taken into + * account here (they have already been read). + * @param len the length of the attribute's content. + * @param buf buffer to be used to call {@link #readUTF8 readUTF8}, + * {@link #readClass(int, char[]) readClass} or {@link #readConst + * readConst}. + * @param codeOff index of the first byte of code's attribute content in + * {@link #b b}, or -1 if the attribute to be read is not a code + * attribute. The 6 attribute header bytes, containing the type + * and the length of the attribute, are not taken into account + * here. + * @param labels the labels of the method's code, or <tt>null</tt> if the + * attribute to be read is not a code attribute. * @return the attribute that has been read, or <tt>null</tt> to skip this - * attribute. + * attribute. Review Comment: Shouldn't this stay indented as it belongs to the `@return` statement on the previous line? ########## grails-common/src/main/groovy/org/apache/grails/common/compiler/asm/TypePath.java: ########## @@ -113,10 +98,9 @@ public int getStep(int index) { * into. This method should only be used for steps whose value is * {@link #TYPE_ARGUMENT TYPE_ARGUMENT}. * - * @param index - * an index between 0 and {@link #getLength()}, exclusive. + * @param index an index between 0 and {@link #getLength()}, exclusive. * @return the index of the type argument that the given step is stepping - * into. + * into. Review Comment: Shouldn't this stay indented as it belongs to the `@return` statement on the previous line? ########## grails-common/src/main/groovy/org/apache/grails/common/compiler/asm/ClassReader.java: ########## @@ -1016,14 +947,12 @@ public String readClass(final int index, final char[] buf) { * method is intended for {@link Attribute} sub classes, and is normally not * needed by class generators or adapters.</i> * - * @param item - * the index of a constant pool item. - * @param buf - * buffer to be used to read the item. This buffer must be - * sufficiently large. It is not automatically resized. + * @param item the index of a constant pool item. + * @param buf buffer to be used to read the item. This buffer must be + * sufficiently large. It is not automatically resized. * @return the {@link Integer}, {@link Float}, {@link Long}, {@link Double}, - * {@link String}, {@link Type} or {@link Handle} corresponding to - * the given constant pool item. + * {@link String}, {@link Type} or {@link Handle} corresponding to + * the given constant pool item. Review Comment: Shouldn't this stay indented as it belongs to the `@return` statement on the previous line? ########## grails-common/src/main/groovy/org/apache/grails/common/compiler/asm/TypePath.java: ########## @@ -98,11 +84,10 @@ public int getLength() { /** * Returns the value of the given step of this path. * - * @param index - * an index between 0 and {@link #getLength()}, exclusive. + * @param index an index between 0 and {@link #getLength()}, exclusive. * @return {@link #ARRAY_ELEMENT ARRAY_ELEMENT}, {@link #INNER_TYPE - * INNER_TYPE}, {@link #WILDCARD_BOUND WILDCARD_BOUND}, or - * {@link #TYPE_ARGUMENT TYPE_ARGUMENT}. + * INNER_TYPE}, {@link #WILDCARD_BOUND WILDCARD_BOUND}, or + * {@link #TYPE_ARGUMENT TYPE_ARGUMENT}. Review Comment: Shouldn't this stay indented as it belongs to the `@return` statement on the previous line? ########## grails-console/src/main/groovy/grails/ui/console/GrailsSwingConsole.groovy: ########## @@ -78,16 +76,16 @@ class GrailsSwingConsole extends GrailsApp { * @param args the application arguments (usually passed from a Java main method) * @return the running {@link org.springframework.context.ApplicationContext} */ - public static ConfigurableApplicationContext run(Class<?>[] sources, String[] args) { - return new GrailsSwingConsole(sources).run(args); + static ConfigurableApplicationContext run(Class<?>[] sources, String[] args) { + return new GrailsSwingConsole(sources).run(args) } /** * Main method to run an existing Application class * * @param args The first argument is the Application class name */ - public static void main(String[] args) { + static void main(String[] args) { if(args) { Review Comment: How come the "space after `if`" didn't get picked up? ########## grails-console/src/main/groovy/grails/ui/shell/GrailsShell.groovy: ########## @@ -73,16 +72,16 @@ class GrailsShell extends GrailsApp { * @param args the application arguments (usually passed from a Java main method) * @return the running {@link org.springframework.context.ApplicationContext} */ - public static ConfigurableApplicationContext run(Class<?>[] sources, String[] args) { - return new GrailsShell(sources).run(args); + static ConfigurableApplicationContext run(Class<?>[] sources, String[] args) { + return new GrailsShell(sources).run(args) } /** * Main method to run an existing Application class * * @param args The first argument is the Application class name */ - public static void main(String[] args) { + static void main(String[] args) { if(args) { Review Comment: Space after `if`? ########## grails-controllers/src/main/groovy/org/grails/compiler/web/ControllerActionTransformer.java: ########## @@ -377,33 +353,24 @@ private MethodNode convertToMethodAction(ClassNode classNode, MethodNode methodN String methodName = methodNode.getName(); String initialValue = param.getInitialExpression().getText(); String methodDeclaration = methodNode.getText(); - String message = "Parameter [%s] to method [%s] has default value [%s]. " + - "Default parameter values are not allowed in controller action methods. ([%s])"; - String formattedMessage = String.format(message, paramName, methodName, - initialValue, methodDeclaration); + String message = "Parameter [%s] to method [%s] has default value [%s]. " + "Default parameter values are not allowed in controller action methods. ([%s])"; + String formattedMessage = String.format(message, paramName, methodName, initialValue, methodDeclaration); GrailsASTUtils.error(source, methodNode, formattedMessage); } } MethodNode method = null; if (methodNode.getParameters().length > 0) { final BlockStatement methodCode = new BlockStatement(); - + final BlockStatement codeToHandleAllowedMethods = getCodeToHandleAllowedMethods(classNode, methodNode.getName()); - final Statement codeToCallOriginalMethod = addOriginalMethodCall(methodNode, initializeActionParameters( - classNode, methodNode, methodNode.getName(), parameters, source, context)); - + final Statement codeToCallOriginalMethod = addOriginalMethodCall(methodNode, initializeActionParameters(classNode, methodNode, methodNode.getName(), parameters, source, context)); + methodCode.addStatement(codeToHandleAllowedMethods); methodCode.addStatement(codeToCallOriginalMethod); - - method = new MethodNode( - methodNode.getName(), - Modifier.PUBLIC, returnType, - ZERO_PARAMETERS, - EMPTY_CLASS_ARRAY, - methodCode); - + method = new MethodNode(methodNode.getName(), Modifier.PUBLIC, returnType, ZERO_PARAMETERS, EMPTY_CLASS_ARRAY, methodCode); Review Comment: Suggestion: ```java method = new MethodNode( methodNode.getName(), Modifier.PUBLIC, returnType, ZERO_PARAMETERS, EMPTY_CLASS_ARRAY, methodCode ); ``` ########## grails-controllers/src/main/groovy/org/grails/compiler/web/ControllerActionTransformer.java: ########## @@ -166,39 +177,21 @@ def someAction() { public class ControllerActionTransformer implements GrailsArtefactClassInjector, AnnotatedClassInjector, CompilationUnitAware { public static final AnnotationNode DELEGATING_METHOD_ANNOATION = new AnnotationNode(ClassHelper.make(DelegatingMethod.class)); - public static Pattern CONTROLLER_PATTERN = Pattern.compile(".+/" + - GrailsResourceUtils.GRAILS_APP_DIR + "/controllers/(.+)Controller\\.groovy"); + public static final Pattern CONTROLLER_PATTERN = Pattern.compile(".+/" + GrailsResourceUtils.GRAILS_APP_DIR + "/controllers/(.+)Controller\\.groovy"); + public static final AnnotationNode ACTION_ANNOTATION_NODE = new AnnotationNode(new ClassNode(Action.class)); + public static final String EXCEPTION_HANDLER_META_DATA_FIELD_NAME = "$exceptionHandlerMetaData"; + public static final String VOID_TYPE = "void"; + + public static final String CONVERT_CLOSURES_KEY = "grails.compile.artefacts.closures.convert"; + private static final String ALLOWED_METHODS_HANDLED_ATTRIBUTE_NAME = "ALLOWED_METHODS_HANDLED"; private static final ClassNode OBJECT_CLASS = new ClassNode(Object.class); - public static final AnnotationNode ACTION_ANNOTATION_NODE = new AnnotationNode( - new ClassNode(Action.class)); private static final String ACTION_MEMBER_TARGET = "commandObjects"; - public static final String EXCEPTION_HANDLER_META_DATA_FIELD_NAME = "$exceptionHandlerMetaData"; private static final TupleExpression EMPTY_TUPLE = new TupleExpression(); @SuppressWarnings({"unchecked"}) - private static final Map<ClassNode, String> TYPE_WRAPPER_CLASS_TO_CONVERSION_METHOD_NAME = CollectionUtils.<ClassNode, String>newMap( - ClassHelper.Integer_TYPE, "int", - ClassHelper.Float_TYPE, "float", - ClassHelper.Long_TYPE, "long", - ClassHelper.Double_TYPE, "double", - ClassHelper.Short_TYPE, "short", - ClassHelper.Boolean_TYPE, "boolean", - ClassHelper.Byte_TYPE, "byte", - ClassHelper.Character_TYPE, "char"); - private static List<ClassNode> PRIMITIVE_CLASS_NODES = CollectionUtils.<ClassNode>newList( - ClassHelper.boolean_TYPE, - ClassHelper.char_TYPE, - ClassHelper.int_TYPE, - ClassHelper.short_TYPE, - ClassHelper.long_TYPE, - ClassHelper.double_TYPE, - ClassHelper.float_TYPE, - ClassHelper.byte_TYPE); - public static final String VOID_TYPE = "void"; - - public static final String CONVERT_CLOSURES_KEY = "grails.compile.artefacts.closures.convert"; - + private static final Map<ClassNode, String> TYPE_WRAPPER_CLASS_TO_CONVERSION_METHOD_NAME = CollectionUtils.<ClassNode, String>newMap(ClassHelper.Integer_TYPE, "int", ClassHelper.Float_TYPE, "float", ClassHelper.Long_TYPE, "long", ClassHelper.Double_TYPE, "double", ClassHelper.Short_TYPE, "short", ClassHelper.Boolean_TYPE, "boolean", ClassHelper.Byte_TYPE, "byte", ClassHelper.Character_TYPE, "char"); + private static final List<ClassNode> PRIMITIVE_CLASS_NODES = CollectionUtils.<ClassNode>newList(ClassHelper.boolean_TYPE, ClassHelper.char_TYPE, ClassHelper.int_TYPE, ClassHelper.short_TYPE, ClassHelper.long_TYPE, ClassHelper.double_TYPE, ClassHelper.float_TYPE, ClassHelper.byte_TYPE); Review Comment: This is worse. Suggestion: ```java private static final Map<ClassNode, String> TYPE_WRAPPER_CLASS_TO_CONVERSION_METHOD_NAME = CollectionUtils.<ClassNode, String>newMap( ClassHelper.Integer_TYPE, "int", ClassHelper.Float_TYPE, "float", ClassHelper.Long_TYPE, "long", ClassHelper.Double_TYPE, "double", ClassHelper.Short_TYPE, "short", ClassHelper.Boolean_TYPE, "boolean", ClassHelper.Byte_TYPE, "byte", ClassHelper.Character_TYPE, "char" ); private static final List<ClassNode> PRIMITIVE_CLASS_NODES = CollectionUtils.newList( ClassHelper.boolean_TYPE, ClassHelper.char_TYPE, ClassHelper.int_TYPE, ClassHelper.short_TYPE, ClassHelper.long_TYPE, ClassHelper.double_TYPE, ClassHelper.float_TYPE, ClassHelper.byte_TYPE ); ``` ########## grails-controllers/src/main/groovy/grails/artefact/Controller.groovy: ########## @@ -369,13 +363,13 @@ trait Controller implements ResponseRenderer, ResponseRedirector, RequestForward try { final DataBindingSource dataBindingSource = DataBindingUtils .createDataBindingSource( - getGrailsApplication(), type, - request) + getGrailsApplication(), type, + request) Review Comment: Is it possible, that when multi-lining method params, to put each param on it's own line and the end parentheses on it's own line indented with the method name? ```groovy .createDataBindingSource( getGrailsApplication(), type, request ) ``` ########## grails-controllers/src/main/groovy/org/grails/compiler/web/ControllerActionTransformer.java: ########## @@ -377,33 +353,24 @@ private MethodNode convertToMethodAction(ClassNode classNode, MethodNode methodN String methodName = methodNode.getName(); String initialValue = param.getInitialExpression().getText(); String methodDeclaration = methodNode.getText(); - String message = "Parameter [%s] to method [%s] has default value [%s]. " + - "Default parameter values are not allowed in controller action methods. ([%s])"; - String formattedMessage = String.format(message, paramName, methodName, - initialValue, methodDeclaration); + String message = "Parameter [%s] to method [%s] has default value [%s]. " + "Default parameter values are not allowed in controller action methods. ([%s])"; + String formattedMessage = String.format(message, paramName, methodName, initialValue, methodDeclaration); GrailsASTUtils.error(source, methodNode, formattedMessage); } } MethodNode method = null; if (methodNode.getParameters().length > 0) { final BlockStatement methodCode = new BlockStatement(); - + final BlockStatement codeToHandleAllowedMethods = getCodeToHandleAllowedMethods(classNode, methodNode.getName()); - final Statement codeToCallOriginalMethod = addOriginalMethodCall(methodNode, initializeActionParameters( - classNode, methodNode, methodNode.getName(), parameters, source, context)); - + final Statement codeToCallOriginalMethod = addOriginalMethodCall(methodNode, initializeActionParameters(classNode, methodNode, methodNode.getName(), parameters, source, context)); + Review Comment: Suggestion: ```java var codeToCallOriginalMethod = addOriginalMethodCall( methodNode, initializeActionParameters( classNode, methodNode, methodNode.getName(), parameters, source, context ) ); ``` ########## grails-controllers/src/main/groovy/org/grails/compiler/web/ControllerActionTransformer.java: ########## @@ -313,25 +298,17 @@ protected boolean methodShouldBeConfiguredAsControllerAction(final MethodNode me //Trait methods have a line number of -1 --minLineNumber; } - return !method.isStatic() && - method.isPublic() && - !method.isAbstract() && - method.getAnnotations(ACTION_ANNOTATION_NODE.getClassNode()).isEmpty() && - method.getAnnotations(new ClassNode(ControllerMethod.class)).isEmpty() && - method.getLineNumber() >= minLineNumber && - !method.getName().startsWith("$") && - !method.getReturnType().getName().equals(VOID_TYPE) && - !isExceptionHandlingMethod(method); + return !method.isStatic() && method.isPublic() && !method.isAbstract() && method.getAnnotations(ACTION_ANNOTATION_NODE.getClassNode()).isEmpty() && method.getAnnotations(new ClassNode(ControllerMethod.class)).isEmpty() && method.getLineNumber() >= minLineNumber && !method.getName().startsWith("$") && !method.getReturnType().getName().equals(VOID_TYPE) && !isExceptionHandlingMethod(method); Review Comment: This is worse. ########## grails-controllers/src/main/groovy/org/grails/compiler/web/ControllerActionTransformer.java: ########## @@ -466,16 +429,13 @@ private void processClosures(ClassNode classNode, SourceUnit source, GeneratorCo } } - protected void addMethodToInvokeClosure(ClassNode controllerClassNode, - PropertyNode closureProperty, SourceUnit source, GeneratorContext context) { + protected void addMethodToInvokeClosure(ClassNode controllerClassNode, PropertyNode closureProperty, SourceUnit source, GeneratorContext context) { MethodNode method = controllerClassNode.getMethod(closureProperty.getName(), ZERO_PARAMETERS); if (method == null || !method.getDeclaringClass().equals(controllerClassNode)) { ClosureExpression closureExpression = (ClosureExpression) closureProperty.getInitialExpression(); final Parameter[] parameters = closureExpression.getParameters(); - final BlockStatement newMethodCode = initializeActionParameters( - controllerClassNode, closureProperty, closureProperty.getName(), - parameters, source, context); + final BlockStatement newMethodCode = initializeActionParameters(controllerClassNode, closureProperty, closureProperty.getName(), parameters, source, context); Review Comment: Use multi-line as above suggestions? ########## grails-controllers/src/main/groovy/org/grails/compiler/web/ControllerActionTransformer.java: ########## @@ -429,8 +396,7 @@ private Statement addOriginalMethodCall(MethodNode methodNode, BlockStatement bl arguments.addExpression(new VariableExpression(p.getName(), p.getType())); } - MethodCallExpression callExpression = new MethodCallExpression( - new VariableExpression("this"), methodNode.getName(), arguments); + MethodCallExpression callExpression = new MethodCallExpression(new VariableExpression("this"), methodNode.getName(), arguments); Review Comment: Suggestion: ```java var callExpression = new MethodCallExpression( new VariableExpression("this"), methodNode.getName(), arguments ); ``` ########## grails-console/src/main/groovy/grails/ui/shell/GrailsShell.groovy: ########## @@ -46,24 +46,23 @@ class GrailsShell extends GrailsApp { configureApplicationContextClass() } - public configureApplicationContextClass() { + def configureApplicationContextClass() { Review Comment: Shouldn't this be `void`? ########## grails-controllers/src/main/groovy/org/grails/compiler/web/ControllerActionTransformer.java: ########## Review Comment: There are a lot of `final` declarations for local variables. Can we remove these? We could also use `var` for these to make this class a lot more readable. ########## grails-controllers/src/main/groovy/org/grails/compiler/web/ControllerActionTransformer.java: ########## @@ -484,12 +444,10 @@ protected void addMethodToInvokeClosure(ClassNode controllerClassNode, } } - final MethodCallExpression methodCallExpression = new MethodCallExpression( - closureExpression, "call", closureInvocationArguments); + final MethodCallExpression methodCallExpression = new MethodCallExpression(closureExpression, "call", closureInvocationArguments); newMethodCode.addStatement(new ExpressionStatement(applyMethodTarget(methodCallExpression, Closure.class, Object.class))); - final MethodNode methodNode = new MethodNode(closureProperty.getName(), Modifier.PUBLIC, - new ClassNode(Object.class), ZERO_PARAMETERS, EMPTY_CLASS_ARRAY, newMethodCode); + final MethodNode methodNode = new MethodNode(closureProperty.getName(), Modifier.PUBLIC, new ClassNode(Object.class), ZERO_PARAMETERS, EMPTY_CLASS_ARRAY, newMethodCode); Review Comment: Multi-line, non-final, var? ########## grails-controllers/src/main/groovy/org/grails/compiler/web/ControllerActionTransformer.java: ########## @@ -484,12 +444,10 @@ protected void addMethodToInvokeClosure(ClassNode controllerClassNode, } } - final MethodCallExpression methodCallExpression = new MethodCallExpression( - closureExpression, "call", closureInvocationArguments); + final MethodCallExpression methodCallExpression = new MethodCallExpression(closureExpression, "call", closureInvocationArguments); Review Comment: Multi-line? ########## grails-controllers/src/main/groovy/org/grails/compiler/web/ControllerActionTransformer.java: ########## @@ -440,9 +406,7 @@ private Statement addOriginalMethodCall(MethodNode methodNode, BlockStatement bl } private boolean isCommandObjectAction(Parameter[] params) { - return params != null && params.length > 0 - && params[0].getType() != new ClassNode(Object[].class) - && params[0].getType() != new ClassNode(Object.class); + return params != null && params.length > 0 && params[0].getType() != new ClassNode(Object[].class) && params[0].getType() != new ClassNode(Object.class); Review Comment: This is worse. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: notifications-unsubscr...@grails.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
