Exception Wrapper #32

Merged
kske merged 3 commits from f/exception-wrapper into develop 2022-01-08 16:54:06 +01:00
Owner

Closes #31

Closes #31
kske self-assigned this 2022-01-08 14:34:04 +01:00
kske added 1 commit 2022-01-08 14:34:05 +01:00
kske requested review from delvh 2022-01-08 14:34:08 +01:00
kske requested review from mpk 2022-01-08 14:34:08 +01:00
delvh requested changes 2022-01-08 14:39:16 +01:00
delvh left a comment
Owner

There should also be a note inside the README about that.

There should also be a note inside the README about that.
@ -215,3 +220,3 @@
* @see Event
*/
public void registerListener(Object listener) throws EventBusException {
public void registerListener(Object listener) {
Owner

Why did you remove that declaration?

Why did you remove that declaration?
Owner

Because we decided against that for our other projects and it also seems to be like this in other big Java projects like Spring. The @throws in the Javadoc will stay, though.

Because we decided against that for our other projects and it also seems to be like this in other big Java projects like Spring. The `@throws` in the Javadoc will stay, though.
Owner

I think that every exception you explicitly declare via throw should also be mentioned in throws.

I know it is unnecessary for unchecked exceptions.

However, I noticed that whenever I'm not directly in a Giga-IDE such as Eclipse, my first course of action is to look for throws in the source code, and not @throws in the Javadoc, especially as it is often faster to open the source code and look there instead of the javadoc as Javadoc always needs a timeout to plop up, and knowing Eclipse, it sometimes even doesn't plop up. And even if it plops up, you still have to read through the Javadoc, which is also longer than simply looking at the source code.

That's why I prefer it that way.

And regarding Spring: Spring is not a project where you should get your styleguides from. Otherwise we also should name our interface implementations <interface>Impl or <interface>Spec. Or have classes such as the MultiPartResolverTestFactoryTest.

I think that every exception you **explicitly** declare via `throw` should also be mentioned in `throws`. I know it is unnecessary for unchecked exceptions. However, I noticed that whenever I'm not directly in a Giga-IDE such as Eclipse, my first course of action is to look for `throws` in the source code, and not `@throws` in the Javadoc, especially as it is often faster to open the source code and look there instead of the javadoc as Javadoc always needs a timeout to plop up, and knowing Eclipse, it sometimes even doesn't plop up. And even if it plops up, you still have to read through the Javadoc, which is also longer than simply looking at the source code. That's why I prefer it that way. And regarding Spring: Spring is **not** a project where you should get your styleguides from. Otherwise we also should name our interface implementations `<interface>Impl` or `<interface>Spec`. Or have classes such as the `MultiPartResolverTestFactoryTest`.
Owner

In that case, however, the code base would already violate compliance, as we don't declare throws NullPointerException for example. Also, we would have to consider all unchecked exceptions potentially thrown by the methods that are called by the method we want to declare throws on, which don't follow those conventions themselved. That would be a large and unnecessary effort in my opinion.

In that case, however, the code base would already violate compliance, as we don't declare `throws NullPointerException` for example. Also, we would have to consider all unchecked exceptions potentially thrown by the methods that are called by the method we want to declare `throws` on, which don't follow those conventions themselved. That would be a large and unnecessary effort in my opinion.
Owner

I think that every exception you explicitly declare via throw should also be mentioned in throws.

> I think that every exception you **explicitly** declare via `throw` should also be mentioned in `throws`.
Owner

Why? This is completely arbitrary. So the NullPointerException should be removed frmo the Javadoc in that case?

Why? This is completely arbitrary. So the `NullPointerException` should be removed frmo the Javadoc in that case?
Owner

I'd say it depends: When you explicitly call Objects.requireNonNull() or something similar, then no.

I've oversimplified my view point in the previous messages.
I am in favor of explicitly declaring unchecked exceptions, when:

  1. you explicitly use throw yourself
  2. you call functions whose main purpose is to check a state (i.e. Objects.requireNonNull)
  3. when there are parameters that you yourself have no restrictions on, but other functions have, and your only option would be to copy the check in the other method to ensure that they don't throw an unchecked exception
    (i.e. you have two string parameters, of which one is is the complete name of an enum class and the other is the name of one of its constants. Then I would declare the exceptions thrown both from Class.forName() and from <enum class>.valueOf() inside throws)
  4. I have to cast where I can only assume that it is the correct type.

I am only in favor of declaring something JVM-internal such as a NullPointerException when you're actually doing a null check, and never else.

I'd say it depends: When you explicitly call `Objects.requireNonNull()` or something similar, then no. I've oversimplified my view point in the previous messages. I am in favor of explicitly declaring unchecked exceptions, when: 1. you explicitly use `throw` yourself 2. you call functions whose main purpose is to check a state (i.e. `Objects.requireNonNull`) 3. when there are parameters that you yourself have no restrictions on, but other functions have, and your only option would be to copy the check in the other method to ensure that they don't throw an unchecked exception (i.e. you have two string parameters, of which one is is the complete name of an enum class and the other is the name of one of its constants. Then I would declare the exceptions thrown both from `Class.forName()` and from `<enum class>.valueOf()` inside `throws`) 4. I have to cast where I can only assume that it is the correct type. I am only in favor of declaring something JVM-internal such as a `NullPointerException` when you're actually doing a null check, and never else.
Owner

If I were to implement this, multiple other methods in the code base have to be adjusted, as well as their Javadoc. Are you in favor of this?

If I were to implement this, multiple other methods in the code base have to be adjusted, as well as their Javadoc. Are you in favor of this?
Owner

But first I'm interested in whether @kske overrules me.

But first I'm interested in whether @kske overrules me.
delvh marked this conversation as resolved
@ -0,0 +1,25 @@
package dev.kske.eventbus.core;
Owner

One blank line too much

One blank line too much
mpk marked this conversation as resolved
mpk added 1 commit 2022-01-08 15:02:37 +01:00
mpk requested review from delvh 2022-01-08 15:02:46 +01:00
mpk approved these changes 2022-01-08 15:03:47 +01:00
delvh requested changes 2022-01-08 15:27:54 +01:00
delvh left a comment
Owner

No matter our opinion on throws on unchecked exceptions, the documentation in the README is still needed.

No matter our opinion on throws on unchecked exceptions, the documentation in the README is still needed.
Owner

No matter our opinion on throws on unchecked exceptions, the documentation in the README is still needed.

Write that shit yourself.

> No matter our opinion on throws on unchecked exceptions, the documentation in the README is still needed. Write that shit yourself.
mpk requested review from delvh 2022-01-08 16:16:43 +01:00
delvh added 1 commit 2022-01-08 16:45:18 +01:00
delvh approved these changes 2022-01-08 16:49:15 +01:00
delvh left a comment
Owner

Write that shit yourself.

I dumped that shit on you. Now review it.

> Write that shit yourself. I dumped that shit on you. Now review it.
@ -0,0 +19,4 @@
* @since 1.2.1
*/
public ExceptionWrapper(Exception cause) {
super(cause);
Owner

What I just noticed: Do we want to allow null causes?

What I just noticed: Do we want to allow `null` causes?
Owner

Well, Event Bus doesn't have a problem with it, even though it makes little sense. I would leave it.

Well, Event Bus doesn't have a problem with it, even though it makes little sense. I would leave it.
delvh marked this conversation as resolved
mpk approved these changes 2022-01-08 16:52:04 +01:00
kske closed this pull request 2022-01-08 16:53:41 +01:00
kske reopened this pull request 2022-01-08 16:53:50 +01:00
kske merged commit 27d14a844d into develop 2022-01-08 16:54:06 +01:00
kske deleted branch f/exception-wrapper 2022-01-08 16:54:06 +01:00
Sign in to join this conversation.
No reviewers
No Milestone
No Assignees
3 Participants
Notifications
Due Date
The due date is invalid or out of range. Please use the format 'yyyy-mm-dd'.

No due date set.

Dependencies

No dependencies set.

Reference: zdm/event-bus#32
No description provided.