Add no-argument Platform.getContextLog()

[HannesWell]

This PR adds the convince Platform.getLog() method that returns an ILog for the bundle of the caller-class and uses a stack-walker to obtain the latter.

Calling Platform.getLog(ThisClass.class) to obtain a log is a very common pattern. Providing Platform.getLog() is not only convenient, it also avoids copy-past-errors were one accidentally passes a class from another bundle and consequently uses those bundles log.

Unfortunately this comes with a little performance penalty because the StackWalker.getCallerClass() is not for free. I added a corresponding note in the java-doc.

This was originally created in eclipse-platform/eclipse.platform.runtime#58 and is recreated here due to the merge of the eclipse.platform.runtime repo.

[ghost]

I am fine with "getContextLog" while i think "getCallerLog" would be more precise (like jdk.internal.reflect.Reflection.getCallerClass()). Both are however not as catchy as "getLog". Therefore i would never actually use getSomethingLog, but use a catchy MyPlugin.getLog() instead.

[vogella]

I would also prefer Platform.getLog(), short method name is better and will help in the adaption. The description can be done via the Javadoc.

[mickaelistria]

short method name is better

I challenge that.

The description can be done via the Javadoc.

I challenge that most people do or like to read Javadoc, and am convinced that most people like APIs that do not require them to read doc better.

I summon "Clean Code" by Bob Martin that discusses naming fields or methods; and has some paragraphs dedicated to discussing when to have more context in names vs sticking to generic. Here it's a case when the generic name is not precise enough for consumers to get a clear understanding of what to expect.
FWIW, with a good IDE like Eclipse IDE, some user typing Platform.getLog would see completion proposal for getContextLog as well.

But maybe the reason why we're discussing all that is because we're stuck to the idea that Platform has to host the method. Maybe it's a hint that we could instead Logs utility class with Logs.forBundle, Logs.forClass, Logs.forCaller...

[merks]

I guess we'll need to repeat the long discussion from the previous PR, because while short is good, descriptive is also good, and misleading is definitely bad. I expect the need to be catchy is quantifiable...

[vogella]

Maybe we just vote and pick the one with the highest votes? Testing if polls work here:

/polls getLog getContextLog getCallerLog

[vogella]

No polls AFAICS possible here, I would vote (assuming I have one +2, one +1 and one 0 vote.

getLog +2
getContextLog 0
getCallerLog +1

[mickaelistria]

Maybe we just vote and pick the one with the highest votes?

Call me snob and elitist, but I'd rather stick to OO best practices documented in literature to help in decision process than gathering opinion of a random crowd that may miss a good part of the context or knowledge to make their decision worthy.

[vogella]

Maybe we just vote and pick the one with the highest votes?

Call me snob and elitist, but I'd rather stick to OO best practices documented in literature to help in decision process than gathering opinion of a random crowd that may miss a good part of the context or knowledge to make their decision worthy.

I call you a snob and elitist on your request. :-)

On a serious note, the current name is fine for me, but I still assume that the current Eclipse developer would use a getLog more likely because that is what we always called such methods, for example in Plugin.getLog().

Again, the current name is fine for me (as the new method comes with a small performance cost, is might be even better to not make its name different to the existing ones)

[jukzi]

getLog 0
LogUtils.getLog +1 (or any other Utility class name)
getContextLog 0
getCallerLog +1

not a single +2 because i would not use any of them - i doubt we need just another getLog().

[mickaelistria]

Plugin.getLog(). which returns the log for this plugin, so the context is explicit from the object we're dereferencing.
Here it's a getLog() from a singleton/static object, so the context is way less obvious, and is even misleading (it gives the impression there is a singleton ILog).

I still stand with my proposal that if we want to invest in better logging, then introducing a Logs utility class (similar to java's Executors one) could bring the best ROI on the long run.

[laeubi]

I would also prefer Platform.getLog(), short method name is better and will help in the adaption. The description can be done via the Javadoc.

I also want to reference Status.error/warn/info here where we also do not extend the method name but use the StackWalker already to compute the calling class.

so the context is way less obvious, and is even misleading (it gives the impression there is a singleton ILog).

Is it relevant if it is a singleton or not? Static access itself is rather bad, see below, beside that getLog(Class<?>) actually do not return a class specific instance as well, so I think one can't know what kind of (fresh new, cached, singleton) such methods return from the name...

still stand with my proposal that if we want to invest in better logging, then introducing a Logs utility class (similar to java's Executors one) could bring the best ROI on the long run.

Better would be more using DI, e.g. E4, DS, ... instead of having even more static, this already works as of today and no code changes are required at all.

[HannesWell]

I am fine with "getContextLog" while i think "getCallerLog" would be more precise (like jdk.internal.reflect.Reflection.getCallerClass()). Both are however not as catchy as "getLog". Therefore i would never actually use getSomethingLog, but use a catchy MyPlugin.getLog() instead.

With getCallerLog() I would first think it is the log of the caller of the method (if used in a method), just like Reflection.getCallerClass() returns the class of the caller of the method. Therefore I would find that name misleading as well.
The problem is that an Activator is not always available and AFAICT the general goal is to reduce the usage of activators in general because they reduce startup performance.

But maybe the reason why we're discussing all that is because we're stuck to the idea that Platform has to host the method. Maybe it's a hint that we could instead Logs utility class with Logs.forBundle, Logs.forClass, Logs.forCaller...

That's a good suggestion. I also tought about default factory methods on the interface. Then one could just call ILog.get(), ILog.get(Class<?>) or ILog.get(Bundle). The first one would be for those that just want an ILog to log something and don't care about details and the other ones for those that want to have more control or better performance (although I think that performance isn't a criterion in the vast majority of cases).
While such static factory default methods are convenient in the first place I somehow have the feeling that there

But while I think providing a utility class and reworking the API to move away from the Platform class is a good idea I think it is out of scope for this change. Furthermore there is some potential to majorly rework the underlying mechanism and I suggest to rework the 'factory'-API when that work is done or during that work, to not hinder that and maybe cause the need to introduce yet another API in the near future.
When such rework is done, the Platform.getLog() are probably deprecated and I don't think there is much different if two or three methods are deprecated, is it?

I would also prefer Platform.getLog(), short method name is better and will help in the adaption. The description can be done via the Javadoc.

I also want to reference Status.error/warn/info here where we also do not extend the method name but use the StackWalker already to compute the calling class.

I suspect the difference there is that the Status class has a much more narrow scope than the Platform class.
As said before, although that I think that Platform.getLog() would fit well to the existing methods, I can understand that it can be more confusing than other names, especially when one is used to the Plugin.getLog() method from activators.

still stand with my proposal that if we want to invest in better logging, then introducing a Logs utility class (similar to java's Executors one) could bring the best ROI on the long run.

Better would be more using DI, e.g. E4, DS, ... instead of having even more static, this already works as of today and no code changes are required at all.

But not everything is a Component or an instance managed by E4, nevertheless it should be considered when making progress here.

If we can agree to not majorly rework the Log-factory API for this change, I would be in favor of the current name Platform.getContextLog() because, considering all arguments, IMHO it is the most suitable name, not the ideal one but the others are not better for me.

[laeubi]

I suspect the difference there is that the Status class has a much more narrow scope than the Platform class.

Just remove the status class to see how "narrow" it is ;-)

If we can agree to not majorly rework the Log-factory API for this change, I would be in favor of the current name Platform.getContextLog() because, considering all arguments, IMHO it is the most suitable name, not the ideal one but the others are not better for me.

Cast your vote here eclipse-platform/.github#23 ;-)

[mickaelistria]

But while I think providing a utility class and reworking the API to move away from the Platform class is a good idea I think it is out of scope for this change.

Anything that discuss possible alternative APIs is in the scope of this change. There is no easy way to undo API addition, so it has to be done as well as possible in 1st place without introducing increments that we won't be able to cleanup later.

the Platform.getLog() are probably deprecated and I don't think there is much different if two or three methods are deprecated, is it?

I think the difference is big, and I believe introducing methods now that we're going to deprecate soon after is adding much more complexity to the process overall. Yes, the addition is harder this way, but the removal won't be necessary. Overall it's more productive than rushing things in.

[akurtakov]

What is the status here?

[HannesWell]

What is the status here?

I would say at the moment it is on hold. Personally I have other things that are more important for me at the moment. And since some other work is intended to be done, I think it would be better to do the basics first and build a new logger API on top of that. I don't want to restrict the basic work that may have to circumvent some non ideal API for that case.

Do you want to keep this PR nonetheless or should I close it?

[akurtakov]

I think it's best to close now if "new logger API" is supposed to be built first.