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.

Sign up for GitHub

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

Jump to bottom

Fix clear year and month field when converting to biblatex #6434

Merged
merged 25 commits into from
May 14, 2020
Merged

Fix clear year and month field when converting to biblatex #6434

merged 25 commits into from
May 14, 2020

Conversation

catarinagomespt
Copy link
Contributor

@catarinagomespt catarinagomespt commented May 6, 2020 •
edited
Loading

Fixes #6224

After Cleanup entries with the selected option -> "Convert to biblatex format (for example, move the value of the 'journal' field to 'journal tile')
Should have two behaviours:

  • if the field date is filled: remains the date and removes the year and month fields;
  • if the field date is blank: sets the date value and removes the year and month fields.
  • Change in CHANGELOG.md described (if applicable)
  • Tests created for changes (if applicable)
  • Manually tested changed features in running JabRef (always required)
  • Screenshots added in PR description (for UI changes)
  • Checked documentation: Is the information available and up to date? If not created an issue at https://github.com/JabRef/user-documentation/issues or, even better, submitted a pull request to the documentation repository.

@Siedlerchr Siedlerchr added the status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers label May 7, 2020
@Siedlerchr Siedlerchr changed the title Fix for issue 6224 Fix clear year and month field when converting to biblatex May 7, 2020
calixtus
calixtus requested changes May 7, 2020
View reviewed changes
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.

Looks good to me. Thank you very much.
Please add a changelog entry and this PR should be mergeable.

@catarinagomespt
Copy link
Contributor Author

catarinagomespt commented May 7, 2020 •
edited
Loading

Looks good to me. Thank you very much.
Please add a changelog entry and this PR should be mergeable.

@calixtus
Done. I changed and pushed Changelog.md file with the description of the issue solved.
Please confirm. Thanks

tobiasdiez
tobiasdiez requested changes May 7, 2020
View reviewed changes
Copy link
Member

@tobiasdiez tobiasdiez left a comment

Choose a reason for hiding this comment

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

Thanks for your contribution. I think there should be one additional check for consistent information before cleaning the month/year field. Moreover, can you please add a unit test for your change. Thanks!

src/main/java/org/jabref/logic/cleanup/ConvertToBiblatexCleanup.java Outdated Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
@tobiasdiez tobiasdiez added the status: changes required Pull requests that are not yet complete label May 7, 2020
@silverhorse
Copy link

ConvertToBiblatexCleanupTest.cleanupWithDateAlreadyPresentDoesNothing() needs to be fixed to assert that year and month are empty when all 3 date, year, month are present. May be add more test as per changes requested. I started fixing the issue at the same time as you @catarinagomespt :)

@catarinagomespt
Copy link
Contributor Author

@tobiasdiez
Summarize:

  1. Added additional check before cleaning month/year field:
  • Check if the date field is filled
  • Check if the date field is equal to year
  • Check if the year field is not empty
  1. Added a unit test for the change.
    Thanks!

@catarinagomespt
Copy link
Contributor Author

Thanks @silverhorse. I already saw the ConvertToBiblatexCleanupTest tests.

calixtus
calixtus reviewed May 8, 2020
View reviewed changes
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.

Just one nitpick, besides that, looks good to me. Again. 😉

src/main/java/org/jabref/logic/cleanup/ConvertToBiblatexCleanup.java Outdated Show resolved Hide resolved
@calixtus calixtus removed the status: changes required Pull requests that are not yet complete label May 8, 2020
koppor
koppor requested changes May 8, 2020
View reviewed changes
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.

Thank you for taking the challange to contribute to JabRef!

I just checked the BibLaTeX manual: It supports having year and month in the date field:

grafik

See also the test case cleanupMovesYearMonthToDate.

Thus, the code should be adapted: If year is present in the date field, copy over the month value to the date field. Log a warning if the month differs. No need for a user warning, because this will be seldom. - Nevertheless, I would question the functionality of JabRef if it silently removes the month value in the case it exists just in the month field.

@koppor koppor removed the status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers label May 8, 2020
@koppor
Copy link
Member

koppor commented May 12, 2020

Tests currently failing. Think @catarinagomespt is working on it.

grafik

@tobiasdiez
Copy link
Member

@catarinagomespt why did you removed the new tests? I think they were pretty good and important to verify that the implementation is also working in certain edge cases.

catarinagomespt
catarinagomespt commented May 12, 2020
View reviewed changes
Copy link
Contributor Author

@catarinagomespt catarinagomespt left a comment

Choose a reason for hiding this comment

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

@tobiasdiez
I put it back.

src/test/java/org/jabref/logic/cleanup/ConvertToBiblatexCleanupTest.java Outdated Show resolved Hide resolved
catarinagomespt
catarinagomespt commented May 12, 2020
View reviewed changes
Copy link
Contributor Author

@catarinagomespt catarinagomespt left a comment

Choose a reason for hiding this comment

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

@tobiasdiez @silverhorse
Now, i check if month or year are different from date do not overwrite/ move, avoiding inconsistent data.

Siedlerchr
Siedlerchr requested changes May 13, 2020
View reviewed changes
Copy link
Member

@Siedlerchr Siedlerchr left a comment

Choose a reason for hiding this comment

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

See my comment, that allows you to simplify your code

src/main/java/org/jabref/model/entry/Date.java Show resolved Hide resolved
src/main/java/org/jabref/logic/cleanup/ConvertToBiblatexCleanup.java Outdated Show resolved Hide resolved
tobiasdiez
tobiasdiez approved these changes May 14, 2020
View reviewed changes
Copy link
Member

@tobiasdiez tobiasdiez left a comment

Choose a reason for hiding this comment

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

Thanks!

@tobiasdiez tobiasdiez merged commit 09111be into JabRef:master May 14, 2020
Siedlerchr added a commit that referenced this pull request May 16, 2020
@Siedlerchr
Merge remote-tracking branch 'upstream/master' into udpate-gradle-to-64
8cd69cc 
* upstream/master: (50 commits)
  Keep group pane size when resizing window (#6180) (#6423)
  Changelog: Fix missing citation for biblatex-mla
  Update AUTHORS
  Check duplicate DOI (#6333)
  Fix missing citation for biblatex-mla
  Change EasyBind dependency (#6480)
  Add testing of latest dev version as mandatory
  Squashed 'src/main/resources/csl-styles/' changes from 5dad23d..586e0b8
  Fix libre office connection and other progress dialogs (#6478)
  Fix clear year and month field when converting to biblatex (#6434)
  Add truncate as a BibTex key modifier (#6427)
  Add new authors (not all - they need more work)
  Remove empty line
  Add simple Unit Tests for #6207 (#6240)
  Enforce LeftCurly rule (#6452)
  Implement task progress indicator (and dialog) in the toolbar (#6443)
  Consider empty brackets
  Changelog update
  Added a test
  Fixed brackets in regular expressions
  ...
koppor pushed a commit that referenced this pull request Mar 1, 2023
Squashed 'buildres/csl/csl-styles/' changes from 9926b48cf3..e9fd2027de
6bf852c 
e9fd2027de Add Medicine Publishing Styles (#6434)
cae128f35f Create Bristol University Press (#6356)
74b4af3b82 Create internet-archaeology.csl (#6357)
ee7ece480b Add Bio-Protocol style (#6429)
9a455efcee Create archives-of-medical-research.csl (#6415)
e91aba46fc Remove some bursa-uludag styles (#6423)
03f3962657 Update offa.csl (#6428)
95dc9b9f5a Update journal-of-neolithic-archaeology.csl (#6427)
a4e6c7f477 Update the-university-of-winchester-harvard.csl (#6374)
c0bf10647a add manuscript formatting to ASA (#6387)
3a673a564a Update universite-de-sherbrooke-histoire.csl (#6392)
0c48c7289e Update chemistry-education-research-and-practice.csl (#6397)
51f718a7b9 Update journal-of-endodontics.csl (#6409)
51e419051f Update presses-universitaires-de-rennes.csl (#6413)

git-subtree-dir: buildres/csl/csl-styles
git-subtree-split: e9fd2027de4e2355f3244ac662960467e225774d
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tobiasdiez tobiasdiez tobiasdiez approved these changes

@Siedlerchr Siedlerchr Siedlerchr approved these changes

@koppor koppor Awaiting requested review from koppor

@calixtus calixtus Awaiting requested review from calixtus

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

allow merging of year and date if identical
6 participants
@catarinagomespt @silverhorse @koppor @tobiasdiez @Siedlerchr @calixtus