Fix Nullwarnings - A#14116
Conversation
|
|
||
| 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
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.
|
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
…ersions/org.glassfish.jersey.core-jersey-server-4.0.0 * upstream/main: (31 commits) Adds a 'regenerate' button to AI chat tab (#12191) (#14191) Chore(deps): Bump jablib/src/main/resources/csl-styles (#14323) Update dependency com.konghq:unirest-modules-gson to v4.6.0 (#14322) Convert fixed-value ComboBoxes to SearchableComboBox (#14083) (#14165) Add support for transliterated citation keys (#13893) Update dependency org.apache.maven.plugins:maven-jar-plugin to v3.5.0 (#14321) Add link to latest development version (#14320) Parameterize tests in AuthorTest and AuthorListTest (#14135) Add AGENTS.md and AI_USAGE_POLICY.md from p5.js (#14316) Fix Nullwarnings - A (#14116) Update io.github.darvil82:terminal-text-formatter from 2.2.0 to 2.3.0c (#14317) Streamline maven repositories (#14315) Fix paths to included Java sources Add forgotten .. Update dependency io.github.darvil82:terminal-text-formatter to v2.3.0c (#14314) Chore(deps): Bump io.github.classgraph:classgraph from 4.8.181 to 4.8.184 in /versions (#14304) Chore(deps): Bump com.fasterxml:aalto-xml in /versions (#14311) Chore(deps): Bump commons-io:commons-io in /versions (#14310) Chore(deps): Bump org.apache.maven.plugins:maven-surefire-plugin (#14298) Disable fetcher-gui-test (#14308) ...
* Fix some null warnings * Remove artifact * Fix nullwarnings in FetcherException ISIDOREFetcher and ResearchGate * Undo newline * Introduce constant * Add link to related classes --------- Co-authored-by: Carl Christian Snethlage <calixtus@users.noreply.github.com> Co-authored-by: Oliver Kopp <kopp.dev@gmail.com>
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)