zdm/event-bus
zdm/event-bus
3
Fork 0
Code Issues 2 Pull Requests Releases 8 Wiki Activity

Exception Wrapper #32

Merged
kske merged 3 commits from f/exception-wrapper into develop 2022-01-08 16:54:06 +01:00
Conversation 1 Commits 3 Files Changed 5 +92 -5
kske commented 2022-01-08 14:34:04 +01:00
Owner
Copy Link

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
Copy Link

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

There should also be a note inside the README about that.
core/src/main/java/dev/kske/eventbus/core/EventBus.java
@ -215,3 +220,3 @@
* @see Event
*/
public void registerListener(Object listener) throws EventBusException {
public void registerListener(Object listener) {
delvh commented 2022-01-08 14:36:52 +01:00
Owner
Copy Link

Why did you remove that declaration?

Why did you remove that declaration?
mpk commented 2022-01-08 15:01:57 +01:00
Owner
Copy Link

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 2022-01-08 15:16:06 +01:00
Owner
Copy Link

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`.
mpk commented 2022-01-08 15:24:50 +01:00
Owner
Copy Link

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 2022-01-08 15:25:30 +01:00
Owner
Copy Link

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`.
mpk commented 2022-01-08 15:47:07 +01:00
Owner
Copy Link

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 2022-01-08 16:04:26 +01:00
Owner
Copy Link

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.
mpk commented 2022-01-08 16:06:23 +01:00
Owner
Copy Link

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?
👍 1
delvh commented 2022-01-08 16:10:25 +01:00
Owner
Copy Link

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
core/src/main/java/dev/kske/eventbus/core/ExceptionWrapper.java
@ -0,0 +1,25 @@
package dev.kske.eventbus.core;
delvh commented 2022-01-08 14:37:25 +01:00
Owner
Copy Link

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
Copy Link

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.
mpk commented 2022-01-08 16:16:31 +01:00
Owner
Copy Link

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
Copy Link

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.
core/src/main/java/dev/kske/eventbus/core/ExceptionWrapper.java
@ -0,0 +19,4 @@
* @since 1.2.1
*/
public ExceptionWrapper(Exception cause) {
super(cause);
delvh commented 2022-01-08 16:35:02 +01:00
Owner
Copy Link

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

What I just noticed: Do we want to allow `null` causes?
mpk commented 2022-01-08 16:50:46 +01:00
Owner
Copy Link

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.
Reviewers
No reviewers
delvh
mpk
Labels
Clear labels   
1

1

  
13

13

  
2

2

  
21

21

  
3

3

  
34

34

  
5

5

  
55

55

  
8

8

  
bug

Something isn't working

  
core

Event Bus Core

  
could have

This doesn't hurt

  
duplicate

This issue or pull request already exists

  
enhancement

New feature or request

  
help wanted

Extra attention is needed

  
must have

This is absolutely necessary

  
proc

Event Bus Proc

  
question

Further information is needed

  
should have

This should be implemented

  
wont have

This won't be implemented (for now)

  
L

Branches where 300 < X <= 600 lines are added and more than one work day is needed

  
M

Branches where 100 < X <=300 lines are added

  
S

Branches where X <= 100 lines are added or less than 30 minutes are needed

  
XL

Branch where X > 600 lines are added and more than two work days are needed

  
bug

Something is not working

  
bugfix

Fixes a bug

  
discussion

This has to be discussed

  
documentation

Related to documentation

  
feature

Introduces a new feature

  
maintenance

Related to project maintenance

  
postponed

Won't be done in the immediate future

  
refactoring

Refactoring code or architectures

  
wontfix

Will not be fixed in this state of development

No Label
1
13
2
21
3
34
5
55
8
bug
core
could have
duplicate
enhancement
help wanted
must have
proc
question
should have
wont have
L
M
S
XL
bug
bugfix
discussion
documentation
feature
maintenance
postponed
refactoring
wontfix
Milestone
Clear milestone
No items
No Milestone
Assignees
Clear assignees
No Assignees
kske
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.
Delete Branch "f/exception-wrapper"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?