Use HTTP constants instead of hard-coded strings

[tareksha]

Content-Type, application/, test/plain, Content-Disposition, Method

Added ExtMediaType to commons-lang for extended MediaType constants
Added "zip" and "tar" constants to builder

Signed-off-by: Tareq Sharafy tareq.sharafy@sap.com

[benoitf]

Hello Tareq and thanks for this cleanup task.

Is it possible to use static import ?
So instead of having launcher.service(HttpMethod.GET
we will have launcher.service(GET

It will be easier to read

Thanks

[skabashnyuk]

+1

[tareksha]

Hi @benoitf,
Please allow me to gently question your approach of static imports here. This code foo(POST) is less formal and seems less convenient than foo(HttpMethod.POST) in my humble opinion. We should think if it is actually trivial for most developers to know all these constants and where they came from, they and not de-facto used like Assert.assertEquals().
What do you think?
thank you

[benoitf]

Hi Tareq,

Sure we can discuss about that.

Now developers are familiar with static import and it's trivial as well
When writing unit tests and mocks you'll now always using static import as well (http://mockito.org/)
you're using verify(mockedList).add("one"); or when(....).
The same for assertEquals (Junit or TestNG)

About finding from where the constant come from, you only have to hit the keyboard on your IDE as well
And probably your IDE may propose the list of constants.

Also to focus on the PR, reading POST in a JAXRS command, we don't need to see the Class directly as we know that value behind.
It's the same for the Class, we don't see when viewing HttpMethod.POST from where is HttpMethod.
It may come from
org.springframework.http.HttpMethod
com.google.appengine.api.urlfetch.HTTPMethod
or javax.ws.rs.HttpMethod

but reader don't need to see as well the full package

[tareksha]

Hi @benoitf

I think we have a misunderstanding regarding the tests. I actually did agrees with you on this in my previous comment, simply because static imports of asserts is the de-facto standard way. Convenient enought.

Regarding our issue. I intentionally mentioned formal in my previous reply, that's why I'm afraid any reliance on IDE capabilities is a questionable tolerance. The majority of the common commit reviewing tools (Gerrit, Git GUI, GitHub itself, ...) are straight forward and do not provide semantic referencing.

Constant name is too short, fully qualified class is too long, so a standard class-field seems sound and even if we do tolerate due to IDE capabilities this won't be major factor because the IDE would distinguish HttpMethod of different packages. I guess Che's code itself is a good reference point, it is written with the common class-field notation.

[benoitf]

yes but short constants like GET, POST with kind of explicit content, they're commonly considered as static import. As in general the class prefix is useless as they will mostly provide the same value
Even spotify clients are using that way :-)
https://github.com/spotify/docker-client/blob/master/src/main/java/com/spotify/docker/client/DefaultDockerClient.java#L105-107

[tareksha]

Hi @benoitf,
I'm not sure if Spotify's docker client is a sound reference point for generalization - it goes to the extreme and imports almost all constants and even methods statically, without exceptions:
https://github.com/spotify/docker-client/blob/master/src/main/java/com/spotify/docker/client/DefaultDockerClient.java#L95-109
The result is pretty much like C code where everything is effectively in public scope.
How common is this approach to an average developer that looks at Che's code?

[benoitf]

I would say that static import is logical when you know the definition of the constant without the class notation.
So we have mainly constants where instead of "GET" we use GET or POST instead of "POST"
or APPLICATION_JSON instead of 'application/json' so it's straightforward. The class prefix is almost useless.

[tareksha]

Hi @benoitf,

I do get your point, let's converge on this issue. Given my previous explanations, do we have real-world project where this is accepted as a cross method for using such constants? let's agree that that spotify isn't, per my last reply. We also keep in mind that the bulk of Che's existing code isn't written this way.

Since the discussion is on a less hard-technical issue I would like to have more than four eyes involved, this should be much beneficial for everyone.

[benoitf]

sure let's vote or let others speak about their thoughts

[skabashnyuk]

@tareqhs plz comment https://dev.eclipse.org/mhonarc/lists/che-dev/msg00554.html

[tareksha]

Hi,
do we have a decision for this PR given the vote results?
@skabashnyuk

[benoitf]

looks like it's developer choice to use static import so it's gonna be accepted

[skabashnyuk]

ok for merge.

[benoitf]

merged manually with a rebase
thank you Tareq !
2d6fa92

[benoitf]

also merged on 4.0 branch with cherry-pick