Assert usage in geode-core

[Jinmei Liao <ji...@pivotal.io>]

Is there a initiative to get spring-core dependency out of geode-core? The
only places we are using spring-core classes in geode-core are those Assert
statements like: Assert.isNull, Assert.notNull etc. Should we try to get
rid of those?

-- 
Cheers

Jinmei

Re: Assert usage in geode-core

[John Blum <jb...@pivotal.io>]

If this is the case, I would encourage the use of a fluent API, e.g.
*AssertJ's* assertions, to replace *Spring* core's capable Assert
<http://docs.spring.io/spring/docs/current/javadoc-api/org/springframework/util/Assert.html>
[1]
class.

Resorting back to the ugly and often brittle conditional blocks at the
beginning of methods, as in...

public <ReturnType> someMethod(Object someArgument) {

  if (*isNotValid*(argument)) {
    throw new IllegalArgumentException(String.format("[%s] argument is not
valid... ", argument));
  }

  ...
}

Would be quite disappointing and make the codebase more difficult to
read/maintain.  Often times, developer's "validation logic" is not even as
elegant as what I have demonstrated above, but even this small snippet is
cumbersome to repeat (defying DRY).

I also think duplicating the *Spring* Assert class in the Geode codebase
would not be a good option either.

My $0.02,

-John

[1] http://docs.spring.io/spring/docs/current/javadoc-
api/org/springframework/util/Assert.html


On Wed, Oct 5, 2016 at 4:45 PM, Jinmei Liao <ji...@pivotal.io> wrote:

> Is there a initiative to get spring-core dependency out of geode-core? The
> only places we are using spring-core classes in geode-core are those Assert
> statements like: Assert.isNull, Assert.notNull etc. Should we try to get
> rid of those?
>
> --
> Cheers
>
> Jinmei
>



-- 
-John
503-504-8657
john.blum10101 (skype)

Re: Assert usage in geode-core

[Dan Smith <ds...@pivotal.io>]

We need to get *all* of the optional dependencies out of the core.
That means anything marked optional in geode-core/build.gradle:
spring-shell, spring-webmvc, jansi, mx4j, etc. Mostly that is a matter
of moving gfsh, the admin REST API, etc. into separate subprojects
(GEODE-818).

Until we do that we're going to keep accidentally using these optional
dependencies in places in the core that aren't really optional at all.

As far as those Assert methods go, they appear to only be used in gfsh
and the admin rest API, and those bits are theoretically optional and
they already transitively depend on spring core. But the only way to
see that is to manually check. We need to get this management code out
of the core!

open> ./gradlew geode-core:findUsage -Djar.name=spring-core
Matches
========
org/apache/geode/management/internal/cli/shell/GfshExecutionStrategy.java
org/apache/geode/management/internal/cli/converters/DirConverter.java
org/apache/geode/management/internal/cli/util/CommentSkipHelper.java
org/apache/geode/management/internal/cli/multistep/CLIMultiStepHelper.java
org/apache/geode/management/internal/cli/remote/RemoteExecutionStrategy.java
org/apache/geode/management/internal/cli/parser/GfshMethodTarget.java
org/apache/geode/management/internal/web/util/ConvertUtils.java
org/apache/geode/management/internal/web/io/MultipartFileResourceAdapter.java
org/apache/geode/management/internal/web/http/converter/SerializableObjectHttpMessageConverter.java
org/apache/geode/management/internal/web/http/ClientHttpRequest.java

On Wed, Oct 5, 2016 at 4:56 PM, Anthony Baker <ab...@pivotal.io> wrote:
> Dan did some work on this:
>
> commit 71eb6bfbc429e7cc226671c99f682ec4fb31115d
> Author: Dan Smith <up...@apache.org>
> Date:   Fri Sep 23 16:40:15 2016 -0700
>
>     GEODE-1934: Removing spring-core dependencies from geode-core
>
>     Remving the spring-core usage in geode-core classes that are not cli or
>     web related. The CLI and web code needs to be split into separate
>     projects. The rest of the geode-core should not depend on spring-core.
>
> I think Dan was fixing the case where a client application depends on geode-core and hit a classpath issue due to an optional dependency on spring.
>
> Anthony
>
>
>> On Oct 5, 2016, at 4:45 PM, Jinmei Liao <ji...@pivotal.io> wrote:
>>
>> Is there a initiative to get spring-core dependency out of geode-core? The
>> only places we are using spring-core classes in geode-core are those Assert
>> statements like: Assert.isNull, Assert.notNull etc. Should we try to get
>> rid of those?
>>
>> --
>> Cheers
>>
>> Jinmei
>

Re: Assert usage in geode-core

[Anthony Baker <ab...@pivotal.io>]

Dan did some work on this:

commit 71eb6bfbc429e7cc226671c99f682ec4fb31115d
Author: Dan Smith <up...@apache.org>
Date:   Fri Sep 23 16:40:15 2016 -0700

    GEODE-1934: Removing spring-core dependencies from geode-core

    Remving the spring-core usage in geode-core classes that are not cli or
    web related. The CLI and web code needs to be split into separate
    projects. The rest of the geode-core should not depend on spring-core.

I think Dan was fixing the case where a client application depends on geode-core and hit a classpath issue due to an optional dependency on spring.

Anthony


> On Oct 5, 2016, at 4:45 PM, Jinmei Liao <ji...@pivotal.io> wrote:
> 
> Is there a initiative to get spring-core dependency out of geode-core? The
> only places we are using spring-core classes in geode-core are those Assert
> statements like: Assert.isNull, Assert.notNull etc. Should we try to get
> rid of those?
> 
> --
> Cheers
> 
> Jinmei