Skip to content
This repository has been archived by the owner on May 7, 2020. It is now read-only.

improve nullness handling on PersistentInbox #5677

Merged
merged 2 commits into from Jun 4, 2018
Merged

improve nullness handling on PersistentInbox #5677

merged 2 commits into from Jun 4, 2018

Conversation

maggu2810
Copy link
Contributor

  • Add NonNullByDefault to ThingRegistryChangeListener
  • Add NonNullByDefault to PersistentInbox
  • Add nullness checks where necessary (AFAIK)

Fixes: #5660

* Add NonNullByDefault to ThingRegistryChangeListener
* Add NonNullByDefault to PersistentInbox
* Add nullness checks where necessary (AFAIK)

Fixes: #5660
Signed-off-by: Markus Rathgeb <maggu2810@gmail.com>
notifyListeners(result, EventType.added);
logger.info("Added new thing '{}' to inbox.", thingUID);
public synchronized boolean add(final @Nullable DiscoveryResult rslt) throws IllegalStateException {
if (rslt == null) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we use discoveryResult instead of rslt?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the Eclipse Nullness checker would be clever (as others) it would not be necessary to introduce another local variable at all.
After returning if the argument is null, the checker should know that all occurrences in that method (at least as the argument is also final) are known to be non-null.
But the checker will complain about...

Sure, I don't care how to name the argument. The method signature is defined by the interface, also the JavaDoc, so the name here that is used for the nullness check at all should not matter.

public Stream<DiscoveryResult> stream() {
return this.discoveryResultStorage.getValues().stream();
return this.discoveryResultStorage.getValues().stream().filter(discoveryResult -> discoveryResult == null);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if you filter that way you will end up in streaming nulls, won't you? Wasn't your idea to filter discoveryResult != null ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

getValues() is declarated that it returns a collection that contains potential null entries. So without the filter this method needs to be changed to a stream that contains nullable entries.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@maggu2810 Yes, but still the filter should be the other way around: discoveryResult != null.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You are surely right 🤣

@maggu2810
Copy link
Contributor Author

I will have a look at the failing tests.

Signed-off-by: Markus Rathgeb <maggu2810@gmail.com>
Copy link
Contributor

@afuechsel afuechsel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

Copy link
Contributor

@kaikreuzer kaikreuzer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks

@kaikreuzer kaikreuzer merged commit 9048a75 into eclipse-archived:master Jun 4, 2018
@maggu2810 maggu2810 deleted the issue5660 branch June 4, 2018 11:20
ermartens pushed a commit to ermartens/smarthome that referenced this pull request Jun 15, 2018
* improve nullness handling on PersistentInbox

* Add NonNullByDefault to ThingRegistryChangeListener
* Add NonNullByDefault to PersistentInbox
* Add nullness checks where necessary (AFAIK)

Fixes: eclipse-archived#5660
Signed-off-by: Markus Rathgeb <maggu2810@gmail.com>
@htreu htreu added this to the 0.10.0 milestone Oct 30, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants