Remove dead panic in Entry.Panic

[alecbz]

Entry.log itself panics when the log level is set to PanicLevel, (and
PanicLevel is always eneabled) so this second panic will never be reached.

---

This change is 

[alecbz]

@sirupsen

[dgsb]

Hello @alecbz thanks for your contribution. However this change would make the code correctness dependant of the Panic specific value. I'm not sure it's a good idea to do that.

[alecbz]

@dgsb Can you elaborate? AFAICT right now this line is never run, so removing it should have no effect. Is there a situation where this line can be reached?

edit: Oh, or are you talking about the test? That test is more-or-less a mirror of the TestEntryPanicf test right above it, except it uses Panic instead of Panicf.

[dgsb]

I mean in term of maintainability, if someday we add a new level like

const (
  EarthQuakeLevel = iota
  PanicLevel

This would make the code incorrect and change its behaviour

[alecbz]

Well, first of all, Panicf doesn't have this direct panic call, so if the idea is that this panic is intentionally there, it should probably be added to Panicf as well. It also panics a different value than the panic inside Entry.log (panics just the string message, instead of the log entry), so it seems like that should also be made consistent.

But more to the point, what would you say the intended behavior should be if someone calls Panic on an EarthQuakeLevel logger? I'd imagine no-oping would be the correct approach, in which case the code as-is would be incorrect if a new EarthQuake level were added.

If the idea is that we want to special-case Panic/Panicf so that it always causes a panic even when used on a hypothetical EarthQuakeLevel logger, then we should probably add code to Panicf to also panic directly (and remove the inner panic from Entry.log, since it would be redundant).

[dgsb]

Ok, let's merge that.

Regarding hypothetical new level and side effect behaviour, you may want tool look at what the Fatal calls do.
It should exit the process whatever the current configured level of the logger is. This is what is expected by the user.

As a personal note, I'd rather not have level at all which trigger such side effects:

• Adding an explicit os.Exit in caller is not that hard and would keep the code base of logrus cleaner and simpler.
• Fatal level call should not used quite often except at application startup

[alecbz]

look at what the Fatal calls do. It should exit the process whatever the current configured level of the logger is. This is what is expected by the user.

Ah yeah, the code there correctly calls entry.Logger.Exit(1) in each of Fatal, Fatalf, and Fatalln.

Forcing a panic in all situations doesn't sound unreasonable, but yeah, if that's the direction we want to go, we should add the call to each of Panic, Panicf, and Panicln, and remove it from the inner Entry.log.