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

Resolve File Creation Issue With Medline/PubMed #8552

Merged
merged 7 commits into from
Mar 16, 2022
Merged

Conversation

addak
Copy link
Contributor

@addak addak commented Mar 9, 2022

Issue : Due to the Fetcher Name being "Medline/PubMed", File creation fails as Medline is treated as a Directory and since It isn't created, the issue is observable.
Fixes #8455

  • 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.

@addak
Copy link
Contributor Author

addak commented Mar 9, 2022

@Siedlerchr A better fix would be to change the name in MedlineFetcher from "Medline/PubMed" to some other appropriate one imho.

public class MedlineFetcher implements IdBasedParserFetcher, SearchBasedFetcher 
.
.
.
 @Override
    public String getName() {
        return "Medline/PubMed";  <--- Change the name
    }

@Siedlerchr
Copy link
Member

The fetcher has been named like that for ages. And there can always be a fetcher with a weird name, so this should be fine,

@addak
Copy link
Contributor Author

addak commented Mar 13, 2022

@Siedlerchr Please review!

@addak addak marked this pull request as ready for review March 13, 2022 17:04
Assertions.assertEquals(result.get(0).getName(), "Springer");
Assertions.assertEquals(result.get(1).getName(), "ArXiv");
Assertions.assertEquals(result.get(2).getName(), "Medline/PubMed");
Copy link
Member

Choose a reason for hiding this comment

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

In general, it is better you compare two collections directly, this makes it easier to spot differences in case of failure.
e.g. you have on the one hand:
expectedFetcherNames = LIst.of(.......)
actualFetcherNames = result.stream.map(Fetcher::getName).toCollection...
assertEquals(expectedFetcherNames,

@@ -60,6 +60,9 @@ public void testWhetherAllFilesAreCreated() throws Exception {

assertTrue(Files.exists(Path.of(tempRepositoryDirectory.toString(), hashCodeQuantum + " - Quantum", "result.bib")));
assertTrue(Files.exists(Path.of(tempRepositoryDirectory.toString(), hashCodeCloudComputing + " - Cloud Computing", "result.bib")));

assertTrue(Files.exists(Path.of(tempRepositoryDirectory.toString(), hashCodeQuantum + " - Quantum", "Medline_PubMed.bib")));
Copy link
Member

Choose a reason for hiding this comment

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

I wonder if it makes sense to convert this test to a parameterized unit test.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We would need multiple "study.yml" files, so... yeah, let me know what you think would be better

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.

overall lgtm, just some ideas for improvement.

@Siedlerchr Siedlerchr added the status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers label Mar 13, 2022
CHANGELOG.md Outdated
@@ -65,6 +65,7 @@ Note that this project **does not** adhere to [Semantic Versioning](http://semve
- We fixed an issue where someone could add a duplicate field in the customize entry type dialog. [#8194](https://github.com/JabRef/jabref/issues/8194)
- We fixed a typo in the library properties tab: "String constants". There, one can configure [BibTeX string constants](https://docs.jabref.org/advanced/strings).
- We fixed an issue when writing a non-UTF-8 encoded file: The header is written again. [#8417](https://github.com/JabRef/jabref/issues/8417)
- We fixed an issue where folder creation during systemic review failed due illegal fetcher name. [#8552](https://github.com/JabRef/jabref/pull/8552)
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
- We fixed an issue where folder creation during systemic review failed due illegal fetcher name. [#8552](https://github.com/JabRef/jabref/pull/8552)
- We fixed an issue where folder creation during systemic literature review failed due to an illegal fetcher name. [#8552](https://github.com/JabRef/jabref/pull/8552)

@addak
Copy link
Contributor Author

addak commented Mar 15, 2022

Hi
Could you please tell me what needs to be properly formatted to resolve the format issue with checkstyle?

 @Test
    public void testWhetherAllFilesAreCreated() throws Exception {
        setUp();
        Crawler testCrawler = new Crawler(getPathToStudyDefinitionFile(), gitHandler, generalPreferences, importFormatPreferences, savePreferences, entryTypesManager, new DummyFileUpdateMonitor());

        testCrawler.performCrawl();

        assertTrue(Files.exists(Path.of(tempRepositoryDirectory.toString(), hashCodeQuantum + " - Quantum")));
        assertTrue(Files.exists(Path.of(tempRepositoryDirectory.toString(), hashCodeCloudComputing + " - Cloud Computing")));

        List<String> filesToAssert = List.of("ArXiv.bib", "Springer.bib", "result.bib", "Medline_PubMed.bib");
        filesToAssert.forEach(
                fileName -> {
                    assertTrue(Files.exists(Path.of(tempRepositoryDirectory.toString(), hashCodeQuantum + " - Quantum", fileName)));
                    assertTrue(Files.exists(Path.of(tempRepositoryDirectory.toString(), hashCodeCloudComputing + " - Cloud Computing", fileName)));
                });
        assertTrue(Files.exists(Path.of(tempRepositoryDirectory.toString(), "studyResult.bib")));
    }

@Siedlerchr
Copy link
Member

@addak The whitepsace was tricky, it was the last element int the list.

@Siedlerchr Siedlerchr merged commit 5b27522 into JabRef:main Mar 16, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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.

Systematic Literature Review Cannot create "Medline/PubMed" file on Linux
3 participants