Fix export and import of MS office day/year/month acessed fields #2862
Conversation
Fix possible NPE in Date parsing Fixes #2859
Siedlerchr
added
status: ready-for-review
| } | ||
| Optional<Date> parsedDateAcesseField = Date.parse(dateAccessed); | ||
| String yearAccessed = parsedDateAcesseField.flatMap(Date::getYear).map(accYear -> accYear.toString()) | ||
| .orElse(null); |
There was a problem hiding this comment.
Instead of using null strings, I would prefer if you use the ifPresent method (since we probably don't want to add fields with null as value anyway).
There was a problem hiding this comment.
The null values are an indicator that no value is present, and in this case it is consistent to the rest of the code. In the addField method there is already a check if( value == null ) return
There was a problem hiding this comment.
Well, I also don't like the rest of the code exactly for this reason. But instead of rewriting everything I would vote for an incremental improvement and since you already converted parts to use optionals... (By the way, BibEntry has the distinction between setField and clearField precisely on similar grounds.)
| return Optional.of(new Date(parsedDate)); | ||
| } catch (DateTimeParseException ignored) { | ||
| // Ignored | ||
| if (dateString != null) { |
There was a problem hiding this comment.
I don't like that null is a valid argument. We try to get rid of null values and should be consistent here. Either way, if(StringUtil.isBlank(dateString)) return Optional.empty(); is probably preferable.
There was a problem hiding this comment.
Either we do a Objects.requireNonNull and leave the checking to the caller, or we use this isBlank Method. Currently it will throw an NPE from the TemporalAccessor.parse method if null is passed.
There was a problem hiding this comment.
+1 for throwing NPE to be consistent with the TemporalAccesor.parse method.
tobiasdiez
removed
the
status: ready-for-review
Throw npe on Date parsing with null
|
Change the code to optionals. Merge it in now |
* upstream/master: (23 commits) Implement #2785: resort groups using drag & drop (#2864) Add Library of Congress as ID-fetcher (#2865) Fix export and import of MS office day/year/month acessed fields (#2862) Adsurl to url (#2861) Update LICENSE.md Update Update LICENSE.md Update license file so that github recognize it properly Improve Issue Template Using a Collapsible Log Area Fix #2852: Improve performance of group filtering. Rename GroupSelector to GroupSidePane Fix #2843: Show information correctly in entry editor Remove old entry editor code related to focus selection Implement feedback Menu Greek Translation (#2836) Relaxed the regex to also match negative timezone formats when parsing pdf annotation dates (#2841) Update localization Remove unnecessary group code (and move remaining settings to preferences) Add Local Maven repo as first lookup resource, to avoid having duplicate libs in gradle and maven Implement #2786: Allow selection of multiple groups ...
* upstream/master: (38 commits) Add link to "feature branch workflow" Support Annotations Created by Foxit (#2878) Fixes jacoco by excluding the fetcher tests from analysis (#2877) Fix entry editor (#2875) update bcprov-jdk15on from 1.56 -> 1.57 update assertj-swing-junit from 3.5.0 -> 3.6.0 update mockito-core from 2.7.22 -> 2.8.9 update jfx from 0.11.0 -> 0.11.1 update google guava from 21.0 -> 22.0 Fix Divide by zero exception if rows is zero in Entry Editor Tab (#2873) Implement #2785: resort groups using drag & drop (#2864) Add Library of Congress as ID-fetcher (#2865) Fix export and import of MS office day/year/month acessed fields (#2862) Adsurl to url (#2861) Update LICENSE.md Update Update LICENSE.md Update license file so that github recognize it properly Improve Issue Template Using a Collapsible Log Area Fix #2852: Improve performance of group filtering. ...
Fix possible NPE in Date parsing
Fixes #2859
- [ ] Screenshots added (for bigger UI changes)- [ ] If you changed the localization: Did you rungradle localizationUpdate?