sbglasius commented on code in PR #15654:
URL: https://github.com/apache/grails-core/pull/15654#discussion_r3279375108
##########
grails-async/plugin/src/main/groovy/grails/async/services/PersistenceContextPromiseDecorator.groovy:
##########
@@ -20,8 +20,8 @@ package grails.async.services
import groovy.transform.CompileStatic
-import grails.persistence.support.PersistenceContextInterceptorExecutor
import grails.async.decorator.PromiseDecorator
+import grails.persistence.support.PersistenceContextInterceptorExecutor
Review Comment:
Why the reformat in this PR.
##########
grails-async/gpars/build.gradle:
##########
@@ -27,6 +27,8 @@ plugins {
id 'org.apache.grails.buildsrc.publish'
id 'org.apache.grails.buildsrc.sbom'
id 'org.apache.grails.gradle.grails-code-style'
+ id 'org.apache.grails.gradle.grails-code-analysis'
+ id 'org.apache.grails.gradle.grails-jacoco'
Review Comment:
I'm commenting on this once: Why did this have to go into this PR. It's all
noise making it almost impossible to review.
##########
grails-async/plugin/src/main/groovy/grails/async/web/AsyncGrailsWebRequest.groovy:
##########
@@ -19,23 +19,25 @@
package grails.async.web
-import groovy.transform.CompileStatic
-import org.grails.web.util.GrailsApplicationAttributes
-import org.grails.web.servlet.mvc.GrailsWebRequest
-import org.springframework.context.ApplicationContext
-import org.springframework.util.Assert
-import org.springframework.web.context.request.async.AsyncWebRequest
-
import java.util.concurrent.atomic.AtomicBoolean
import java.util.function.Consumer
+import groovy.transform.CompileStatic
Review Comment:
Import order noise unrelated to this PR changes.
##########
grails-data-hibernate7/dbmigration-core/src/test/resources/logback.groovy:
##########
@@ -0,0 +1,38 @@
+/*
+ * 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.
+ */
+// See http://logback.qos.ch/manual/groovy.html for details on configuration
+def CONSOLE_LOG_PATTERN = '%d{HH:mm:ss.SSS} [%t] %highlight(%p)
%cyan(\\(%logger{39}\\)) %m%n'
Review Comment:
Is logback.groovy even supported?
##########
grails-data-hibernate5/dbmigration/src/test/resources/logback.groovy:
##########
@@ -20,6 +20,7 @@
def CONSOLE_LOG_PATTERN = '%d{HH:mm:ss.SSS} [%t] %highlight(%p)
%cyan(\\(%logger{39}\\)) %m%n'
appender('STDOUT', ConsoleAppender) {
+ follow = true
Review Comment:
Is logback.groovy even supported?
##########
grails-data-mongodb/core/src/test/groovy/org/apache/grails/data/mongo/core/GrailsDataMongoTckManager.groovy:
##########
@@ -18,40 +18,37 @@
*/
package org.apache.grails.data.mongo.core
-import groovy.util.logging.Slf4j
-
import com.mongodb.BasicDBObject
import com.mongodb.client.MongoClient
-import org.bson.Document
-import org.slf4j.LoggerFactory
-import org.testcontainers.containers.MongoDBContainer
-import org.testcontainers.containers.output.Slf4jLogConsumer
-
-import org.springframework.context.support.GenericApplicationContext
-import org.springframework.context.support.StaticMessageSource
-import org.springframework.validation.Validator
-
import grails.core.DefaultGrailsApplication
import grails.core.GrailsApplication
import grails.gorm.validation.PersistentEntityValidator
+import groovy.util.logging.Slf4j
import org.apache.grails.data.testing.tck.base.GrailsDataTckManager
import org.apache.grails.testing.mongo.AbstractMongoGrailsExtension
+import org.bson.Document
import org.grails.datastore.bson.query.BsonQuery
import org.grails.datastore.gorm.GormEnhancer
import org.grails.datastore.gorm.mongo.Birthday
import
org.grails.datastore.gorm.validation.constraints.eval.DefaultConstraintEvaluator
import
org.grails.datastore.gorm.validation.constraints.registry.DefaultConstraintRegistry
-import org.grails.datastore.mapping.core.DatastoreUtils
import org.grails.datastore.mapping.core.Session
+import org.grails.datastore.mapping.multitenancy.MultiTenancySettings
+import
org.grails.datastore.mapping.multitenancy.resolvers.SystemPropertyTenantResolver
import
org.grails.datastore.mapping.engine.types.AbstractMappingAwareCustomTypeMarshaller
import org.grails.datastore.mapping.model.MappingContext
import org.grails.datastore.mapping.model.PersistentEntity
import org.grails.datastore.mapping.model.PersistentProperty
+import org.grails.datastore.mapping.core.DatastoreUtils
import org.grails.datastore.mapping.mongo.MongoDatastore
import org.grails.datastore.mapping.mongo.config.MongoSettings
-import org.grails.datastore.mapping.multitenancy.MultiTenancySettings
-import
org.grails.datastore.mapping.multitenancy.resolvers.SystemPropertyTenantResolver
import org.grails.datastore.mapping.query.Query
+import org.slf4j.LoggerFactory
Review Comment:
Import re-order noise
##########
grails-data-hibernate5/core/src/test/groovy/grails/gorm/specs/WhereQueryBugFixSpec.groovy:
##########
@@ -16,7 +16,7 @@
* specific language governing permissions and limitations
* under the License.
*/
-package grails.gorm.tests
+package grails.gorm.specs
Review Comment:
10% of files reviewed... 🤔
##########
grails-data-hibernate5/core/src/test/groovy/grails/gorm/hibernate/mapping/HibernateMappingBuilderTests.groovy:
##########
@@ -20,12 +20,8 @@ package grails.gorm.hibernate.mapping
import org.grails.orm.hibernate.cfg.CompositeIdentity
import org.grails.orm.hibernate.cfg.HibernateMappingBuilder
-
-/**
- * Created by graemerocher on 01/02/2017.
- */
-
Review Comment:
Removing it is nice, just not in this PR.
##########
grails-data-mongodb/core/src/test/groovy/org/apache/grails/data/mongo/core/MongoDatastoreSpec.groovy:
##########
@@ -0,0 +1,24 @@
+/*
+ * 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.
+ */
+package org.apache.grails.data.mongo.core
+
+import org.apache.grails.data.testing.tck.base.GrailsDataTckSpec
+
+abstract class MongoDatastoreSpec extends
GrailsDataTckSpec<GrailsDataMongoTckManager> {
Review Comment:
This refactor should have been done in a later PR. Adding to the size of the
PR.
##########
grails-data-hibernate5/core/src/test/groovy/grails/gorm/specs/autoimport/other/A.groovy:
##########
@@ -17,7 +17,7 @@
* under the License.
*/
-package grails.gorm.tests.autoimport.other
+package grails.gorm.specs.autoimport.other
Review Comment:
Noice
##########
grails-async/core/src/main/groovy/org/grails/async/transform/internal/DelegateAsyncTransformation.java:
##########
@@ -200,7 +201,7 @@ protected DelegateAsyncTransactionalMethodTransformer
lookupAsyncTransactionalMe
try {
Class<?> transformerClass =
getClass().getClassLoader().loadClass("org.grails.async.transform.internal.DefaultDelegateAsyncTransactionalMethodTransformer");
return (DelegateAsyncTransactionalMethodTransformer)
transformerClass.getDeclaredConstructor().newInstance();
- } catch (Throwable e) {
+ } catch (Exception ignored) {
Review Comment:
One explanation is that it's bad coding style to catch Throwables. And
`ignored` is an IntelliJ thing 🤔
##########
grails-data-mongodb/core/src/test/groovy/grails/mongodb/cascade/MongoCascadeSpec.groovy:
##########
@@ -18,12 +18,13 @@
*/
package grails.mongodb.cascade
+import org.apache.grails.data.mongo.core.MongoDatastoreSpec
+
import org.apache.grails.data.mongo.core.GrailsDataMongoTckManager
-import org.apache.grails.data.testing.tck.base.GrailsDataTckSpec
-class MongoCascadeSpec extends GrailsDataTckSpec<GrailsDataMongoTckManager> {
+class MongoCascadeSpec extends MongoDatastoreSpec {
Review Comment:
Was a refactor of `GrailsDataTckSpec<GrailsDataMongoTckManager>` to
`MongoDatastoreSpec` needed? If yes, ok, if no, then it's just another source
of noise.
##########
grails-datamapping-tck/src/main/groovy/org/apache/grails/data/testing/tck/tests/DetachedCriteriaSpec.groovy:
##########
@@ -20,19 +20,23 @@ package org.apache.grails.data.testing.tck.tests
import grails.gorm.DetachedCriteria
import grails.gorm.PagedResultList
-import org.apache.grails.data.testing.tck.base.GrailsDataTckSpec
import org.apache.grails.data.testing.tck.domains.Person
+import org.apache.grails.data.testing.tck.base.GrailsDataTckSpec
class DetachedCriteriaSpec extends GrailsDataTckSpec {
+ void setupSpec() {
+ manager.addAllDomainClasses([Person])
+ }
+
void 'Test the list method returns a PagedResultList with pagination
arguments'() {
given: 'A bunch of people'
createPeople()
when: 'A detached criteria instance is created and the list method
used with the max parameter'
def criteria = new DetachedCriteria(Person)
criteria.with {
- eq('lastName', 'Simpson')
+ eq 'lastName', 'Simpson'
Review Comment:
This is an unneeded change for this PR. Removing parentheses should have
been done in a clean-up PR.
##########
grails-data-docs/guide-developer/src/main/docs/stepByStep.adoc:
##########
@@ -151,7 +151,7 @@ package org.grails.datastore.gorm
import org.junit.runners.Suite.SuiteClasses
import org.junit.runners.Suite
import org.junit.runner.RunWith
-import grails.gorm.tests.*
+import grails.gorm.specs.*
Review Comment:
agreed. It's noise
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
