Exception Wrapper #32

Merged
kske merged 3 commits from f/exception-wrapper into develop 7 months ago
kske commented 7 months ago
Owner

Closes #31

Closes #31
kske self-assigned this 7 months ago
kske added 1 commit 7 months ago
kske requested review from delvh 7 months ago
kske requested review from DieGurke 7 months ago
delvh requested changes 7 months ago
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.
* @see Event
*/
public void registerListener(Object listener) throws EventBusException {
public void registerListener(Object listener) {
delvh commented 7 months ago
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.
delvh commented 7 months ago
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.
delvh commented 7 months ago
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?
delvh commented 7 months ago
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?
delvh commented 7 months ago
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
package dev.kske.eventbus.core;
delvh commented 7 months ago
Owner

One blank line too much

One blank line too much
DieGurke marked this conversation as resolved
DieGurke added 1 commit 7 months ago
DieGurke requested review from delvh 7 months ago
DieGurke approved these changes 7 months ago
delvh requested changes 7 months ago
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.
DieGurke requested review from delvh 7 months ago
delvh added 1 commit 7 months ago
delvh approved these changes 7 months ago
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.
* @since 1.2.1
*/
public ExceptionWrapper(Exception cause) {
super(cause);
delvh commented 7 months ago
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
DieGurke approved these changes 7 months ago
kske closed this pull request 7 months ago
kske reopened this pull request 7 months ago
kske merged commit 27d14a844d into develop 7 months ago
kske deleted branch f/exception-wrapper 7 months ago

Reviewers

delvh approved these changes 7 months ago
DieGurke approved these changes 7 months ago
The pull request has been merged as 27d14a844d.
Sign in to join this conversation.
No reviewers
No Milestone
No Assignees
3 Participants
Notifications
Due Date

No due date set.

Dependencies

No dependencies set.

Reference: zdm/event-bus#32
Loading…
There is no content yet.