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

Exception shouldn't happen when pasting an entry with a publication date-range of form yyyy-yyyy #8247

Merged
merged 17 commits into from
Dec 14, 2021
Merged

Exception shouldn't happen when pasting an entry with a publication date-range of form yyyy-yyyy #8247

merged 17 commits into from
Dec 14, 2021

Conversation

jiezheng5
Copy link
Contributor

@jiezheng5 jiezheng5 commented Nov 15, 2021 •
edited
Loading

Fixes: #7864

  • Change in CHANGELOG.md described in a way that is understandable for the average user (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, I created an issue at https://github.com/JabRef/user-documentation/issues or, even better, I submitted a pull request to the documentation repository.

Siedlerchr
Siedlerchr requested changes Nov 19, 2021
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.

Can you please write a test for this? And also what happens to some alias fields like month etc?
When I see this right, you just save the second date, but otherwise it's ignored?

src/main/java/org/jabref/model/entry/Date.java Outdated
*
* @param date the start date
* @param endDate the start date
* @throws NullPointerException if DOI/Short DOI is null
Copy link
Member

Choose a reason for hiding this comment

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

DOI? Seems like you copied the wrong comments

Copy link
Contributor Author

@jiezheng5 jiezheng5 Nov 21, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

  1. My bad, yes, I copied the wrong java comments.
  2. Do you have any suggestions where to put the JUnit test, or should I create a new .java file for this test?
  3. "And also what happens to some alias fields like month etc?
    When I see this right, you just save the second date, but otherwise it's ignored?"
    if it is the format mm/uuuu, it is still parsed as month/year
    if it is the format uuuu/uuuu, it is parsed as year range
    Your understanding is 100% correct.

Thanks so much for the detailed review.

Copy link
Member

@k3KAW8Pnf7mkmdSMPHz27 k3KAW8Pnf7mkmdSMPHz27 Nov 22, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

  2. In this case, the easiest is to search for the file DateTest (located in src/test/java/org/jabref/model/entry/DateTest.java). If you ever need to create a completely new test file I'd suggest the IntelliJ short-cut, the default option follows the same naming convenient that we are using.
  3. Not sure that I understand, but I am guessing there is no question here?

Copy link
Member

Choose a reason for hiding this comment

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

There's also some more information on the test conventions we are following in https://devdocs.jabref.org/getting-into-the-code/code-howtos#test-cases

Copy link
Contributor Author

Choose a reason for hiding this comment

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

thanks very much for the suggestions.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I removed the improper comments added the unit test.
I really appreciate all the constructive and detailed feedback. I felt very lucky to choose JabRef as my first open-source project. Everyone is so nice and helpful.

@Siedlerchr Siedlerchr added the status: changes required Pull requests that are not yet complete label Nov 19, 2021
@jiezheng5
Copy link
Contributor Author

I added the requested Junit test for the modified parse function in Date.java, and please let me know if there is anything else I should change.

Siedlerchr
Siedlerchr approved these changes Dec 14, 2021
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.

Thanks a lot for your PR!

@Siedlerchr Siedlerchr merged commit 1eed985 into JabRef:main Dec 14, 2021
Siedlerchr added a commit that referenced this pull request Dec 17, 2021
@Siedlerchr
Merge remote-tracking branch 'upstream/main' into tinylog
cc924e0 
* upstream/main:
  Don't register any database changes to the indexer while dropping a file (#8334)
  Fix ACM fetcher (#8338)
  Squashed 'buildres/csl/csl-styles/' changes from 3bb4b5f..60bf7d5
  Exception shouldn't happen when pasting an entry with a publication date-range of form yyyy-yyyy (#8247)
  Refactor Sidepane logic (#8202)
  New translations JabRef_en.properties (Japanese) (#8331)
  Bump bcprov-jdk15on from 1.69 to 1.70 (#8333)
  Update Controlsfx to 11.1.1 (#8330)
  Update citeproc (#8329)
  Bump classgraph from 4.8.137 to 4.8.138 (#8327)
  Bump junit-platform-launcher from 1.8.1 to 1.8.2 (#8326)
  Remove outdated gradle deps check (#8324)
  Bump junit-jupiter from 5.8.1 to 5.8.2 (#8325)
  Bump libreoffice from 7.2.2 to 7.2.3 (#8328)
  Remove outdated ignores
Siedlerchr added a commit that referenced this pull request Dec 20, 2021
@Siedlerchr
Merge remote-tracking branch 'upstream/main' into jabrefonline
2762345 
* upstream/main: (46 commits)
  New Crowdin updates (#8349)
  Bump pdfbox from 2.0.24 to 2.0.25 (#8345)
  Bump fontbox from 2.0.24 to 2.0.25 (#8348)
  Bump xmlunit-core from 2.8.3 to 2.8.4 (#8347)
  Bump tika-core from 2.1.0 to 2.2.0 (#8346)
  Added missing executable bindings to various commands (#8342)
  Update Gradle Wrapper from 7.3.1 to 7.3.2. (#8343)
  Fix issues with writing metadata to pdfs (#8332)
  add tinylog test (#8339)
  Tinylog (#8226)
  Don't register any database changes to the indexer while dropping a file (#8334)
  Fix ACM fetcher (#8338)
  Squashed 'buildres/csl/csl-styles/' changes from 3bb4b5f..60bf7d5
  Exception shouldn't happen when pasting an entry with a publication date-range of form yyyy-yyyy (#8247)
  Refactor Sidepane logic (#8202)
  New translations JabRef_en.properties (Japanese) (#8331)
  Bump bcprov-jdk15on from 1.69 to 1.70 (#8333)
  Update Controlsfx to 11.1.1 (#8330)
  Update citeproc (#8329)
  Bump classgraph from 4.8.137 to 4.8.138 (#8327)
  ...

# Conflicts:
#	build.gradle
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@k3KAW8Pnf7mkmdSMPHz27 k3KAW8Pnf7mkmdSMPHz27 k3KAW8Pnf7mkmdSMPHz27 left review comments

@calixtus calixtus calixtus approved these changes

@Siedlerchr Siedlerchr Siedlerchr approved these changes

Assignees
No one assigned
Labels
status: changes required Pull requests that are not yet complete
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Exception when pasting an entry with a publication date-range of the form 1910/1917
4 participants
@jiezheng5 @Siedlerchr @k3KAW8Pnf7mkmdSMPHz27 @calixtus