Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add BibLatex date formats for parsing to Date.java #9720

Merged

Conversation

GuyPuts
Copy link
Contributor

@GuyPuts GuyPuts commented Mar 31, 2023

contributes to #2753

Adds date formats set by BibLatex to the Date.java class. Formats are included in the list that was already present in the class. Tested locally.

Compulsory checks

Preview Give feedback

Adds date formats set by BibLatex to the Date.java class. Formats are included in the list that was already present in the class. Tested locally.
@Siedlerchr
Copy link
Member

Thanks for your contribution. Please link to the issue with e.g Fixes #.... and also please add tests and a changelog entry

Copy link
Member

@calixtus calixtus left a comment

Choose a reason for hiding this comment

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

Please add a changelog entry and most importantly add tests. Thanks!

Comment on lines 50 to 56
"y-M/y-M", // covers 2015-01/2015-02
"MMMM y/MMMM y", // covers January 2015/February 2015
"d MMMM y/d MMMM y", // covers 20 January 2015/20 February 2015
"y G", // covers 1 BC
"y G / y G", // covers 30 BC / 5 AD
"yyyy G / yyyy G", // covers 0030 BC / 0005 AD
"yyyy-MM G / yyyy-MM G", // covers 0030-01 BC / 0005-02 AD
Copy link
Member

Choose a reason for hiding this comment

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

These date ranges are not yet parsed correctly. The static parse method must probably be adapted somehow.

Copy link
Member

Choose a reason for hiding this comment

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

Yes, see https://docs.oracle.com/en/java/javase/19/docs/api/java.base/java/time/format/DateTimeFormatter.html
and

// if dateString has format of uuuu/uuuu, treat as date range
if (dateString.matches("[0-9]{4}/[0-9]{4}")) {

There are already existing unit tests which you need to extend.

@Siedlerchr Siedlerchr added the status: changes required Pull requests that are not yet complete label Apr 1, 2023
@GuyPuts
Copy link
Contributor Author

GuyPuts commented Apr 22, 2023

PR was updated

@koppor
Copy link
Member

koppor commented Apr 29, 2023

@GuyPuts After pushing, you should check the output of the tests going on at GitHub. I put you a screenshot:

image

It says, checkstyle failed. You can go to the files tab to see the errors https://github.com/JabRef/jabref/pull/9720/files

Alternateively, you can execute checkstyle in your ide as described at https://devdocs.jabref.org/getting-into-the-code/guidelines-for-setting-up-a-local-workspace/intellij-13-code-style.html

Copy link
Member

@koppor koppor left a comment

Choose a reason for hiding this comment

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

I think besides the small comments, this is good to go?

CHANGELOG.md Outdated Show resolved Hide resolved
Siedlerchr added 2 commits May 6, 2023 19:21
…biblatex-date-formats

* upstream/main: (132 commits)
  Add four rules not having any effect
  Apply BooleanChecksNotInverted
  Remove unused RadioButtonCell
  Minimal config for openRewrite
  Fix missing #
  Fix modernizer (JabRef#9824)
  CHANGELOG.md
  Removed unused code
  Refined ui
  Dissolved FileTab and moved contents to EntryTab
  Renamed CustomEditorFieldsTab to EntryEditorTabsTab
  Fixed antipattern, fixed radiobutton with checkbox
  Renamed ImportExportPreferences to ExportPreferences
  Improve search history by attaching change listener (JabRef#9794)
  Separated WebSearchPrefs and ExportPrefs
  Separated WebSearchTab and ExportTab
  Renamed ImportExportTab to WebSearchTab
  Fix split multiline localization (JabRef#9814)
  New Crowdin updates (JabRef#9834)
  change versin to 0.8.10
  ...
@Siedlerchr Siedlerchr added status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers and removed status: changes required Pull requests that are not yet complete labels May 6, 2023
@koppor
Copy link
Member

koppor commented May 7, 2023

DateCheckerTest complains about failing tests

Copy link
Member

@koppor koppor left a comment

Choose a reason for hiding this comment

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

Since the tests fail, something should be done to get them running.

@Siedlerchr Siedlerchr removed the status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers label May 7, 2023
rather add a todo
@Siedlerchr Siedlerchr added the status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers label May 7, 2023
@Siedlerchr
Copy link
Member

Follow up task (n oted in the issue)
/* TODO: The following date formats do not yet work and need to be created with tests
* "u G", // covers 1 BC
* "u G / u G", // covers 30 BC / 5 AD
* "uuuu G / uuuu G", // covers 0030 BC / 0005 AD
* "uuuu-MM G / uuuu-MM G", // covers 0030-01 BC / 0005-02 AD
* "u'-'", // covers 2015-
* "u'?'", // covers 2023?
*/

@Siedlerchr Siedlerchr merged commit 44564ad into JabRef:main May 7, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bib(la)tex status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants