Fix Nullwarnings - A #14116
Fix Nullwarnings - A #14116
Conversation
calixtus
added
the
dev: code-quality
|
|
||
| applyFilter(request.filter()).forEach(id -> { | ||
| EmbeddingRecord eRecord = embeddingsMap.get(id); | ||
| EmbeddingRecord eRecord = embeddingsMap.getOrDefault(id, new EmbeddingRecord(null, "", new float[0])); |
There was a problem hiding this comment.
We could think about a NULL_OBJECT constant
There was a problem hiding this comment.
NULL_OBJECT would also help here to avoid memory increase on all executions of this line. Reason: The fall-back object is created on all calls, even propbably never (!) used. --> we never had NPEs here, had we?
There was a problem hiding this comment.
Is this needed somewhere else?
koppor
left a comment
koppor
left a comment
There was a problem hiding this comment.
I think, we should discuss the comments here, get this in and do other null fixes later. The fixes should get in ASAP - and not pile up.
|
|
||
| applyFilter(request.filter()).forEach(id -> { | ||
| EmbeddingRecord eRecord = embeddingsMap.get(id); | ||
| EmbeddingRecord eRecord = embeddingsMap.getOrDefault(id, new EmbeddingRecord(null, "", new float[0])); |
There was a problem hiding this comment.
NULL_OBJECT would also help here to avoid memory increase on all executions of this line. Reason: The fall-back object is created on all calls, even propbably never (!) used. --> we never had NPEs here, had we?
| DAY("day"), | ||
| DAYFILED("dayfiled"), | ||
| DOI("doi", "DOI", FieldProperty.VERBATIM, FieldProperty.IDENTIFIER), | ||
| DOI("doi", FieldProperty.VERBATIM, FieldProperty.IDENTIFIER), |
There was a problem hiding this comment.
OMG, maybe you found a core issue in JabRef's code here?
With #13867 it seems a large architectural change has been introduced. org.jabref.model.entry.field.FieldTextMapper is now used instead of .getDisplayName(). We should decide whether we want to have the DisplayName close to the field or at some other class. - I first thought that org.jabref.model.entry.field.FieldTextMapper was UI, but it is logic (because of usage by OO code).
I am a bit undecided where to put. I found the "old" way to have it annoted at the field itself better. One could argue to have the field information more lightweight and display is another concern. --> We should link org.jabref.model.entry.field.FieldTextMapper in StandardField
There was a problem hiding this comment.
We had the discussion if I recall correctly. We opted then for a clean separation of ui layer and internal representation.
github-actions
bot
added
the
status: changes-required
|
Im okay with merging asap. We can make a series of null fix prs. Maybe also nice good first issues? Needs some thinking though. |
|
Will introduce null obj constant later today. After that rfr. |
| protected MVStore mvStore; | ||
|
|
||
| public MVStoreBase(@Nullable Path path, NotificationService dialogService) { | ||
| public MVStoreBase(@NonNull Path path, NotificationService dialogService) { |
There was a problem hiding this comment.
Hmm, but didn't it was nullable on purpose?
There was a problem hiding this comment.
Compiler says now "no". 😄
But seriously: I see in calling methods only call to getAiFilesDirectory
which should always be not null:
On the otherside inside the method contract of Files.createDirectories Path argument is NotNull
calixtus
added
the
status: ready-for-review
github-actions
bot
removed
the
status: changes-required
Self describing.
Removed also artifact from #13867
Feel free to pick some classes and push some commits with null fixes to this PR.
Steps to test
Run gradle :jablib:build, see if null warnings have bin fixed.
Mandatory checks
CHANGELOG.mdin a way that is understandable for the average user (if change is visible to the user)