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

Predatory journal checker #10592

Merged
merged 39 commits into from
Dec 5, 2023
Merged

Conversation

xuanan20020
Copy link
Contributor

@xuanan20020 xuanan20020 commented Oct 27, 2023

Continue to resolve koppor#348

The implementation attempts to match the architecture of the JournalInAbbreviationListChecker.

This draft pull request will be updated as the implementation is completed.

Mandatory checks

  • 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 developer's documentation: Is the information available and up to date? If not, I outlined it in this pull request.
  • 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 changed the title Fix for issue 348 Predatory journal checker Oct 28, 2023
@xuanan20020
Copy link
Contributor Author

Thanks for the comments! I will get those issues resolved

@xuanan20020
Copy link
Contributor Author

Sorry, I didn't follow the background thread approach since generating an MV file for predatory journals in the JournalListMvGenerator makes more sense to me, although that abuses the purpose of that class a little. But with this, I just have to update the file once and all subsequent loadrepository() call does not require any update from the web. Also, it was really difficult to get the BackgroundTask Wrapper to work on the update method without failing the MainArchitectureTest

@xuanan20020 xuanan20020 marked this pull request as ready for review October 29, 2023 03:48
@Siedlerchr
Copy link
Member

Siedlerchr commented Nov 1, 2023

Okay, I will try to find a way to integrate this into a BackgroundTask.

* upstream/main:
  Fix CHANGELOG.md
  Bump commons-cli:commons-cli from 1.5.0 to 1.6.0 (JabRef#10607)
  Fix issue JabRef#9306: Move "Open only one instance of JabRef" preference option to somewhere else (JabRef#10602)
  Update README.md (JabRef#10604)
  Bump me.champeau.jmh from 0.7.1 to 0.7.2
  Bump org.beryx.jlink from 3.0.0 to 3.0.1
  Bump com.dlsc.gemsfx:gemsfx from 1.82.0 to 1.84.0
  Bump org.apache.logging.log4j:log4j-to-slf4j from 2.21.0 to 2.21.1

# Conflicts:
#	src/main/resources/l10n/JabRef_en.properties
@Siedlerchr
Copy link
Member

Sorry about that BackgroundTask stuff. I did not know the task is only executed when you run the journal list generation gradle task.

We still need to have an option to update this, but I think this is for future use. currently it will be updated only at build time

Siedlerchr
Siedlerchr previously approved these changes Nov 1, 2023
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.

I am okay with this now. It will be updated along when our github workflow runs. So no need to threading handling

@Siedlerchr Siedlerchr requested review from koppor and calixtus November 1, 2023 12:13
@xuanan20020
Copy link
Contributor Author

Thanks for reviewing the code :D. It was @koppor 's suggestion to have the MV file generated in such a manner. He recommended under the issue.

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 went through the code and have detail comments.

But one general one, which I was not able to fully write the new code.

The core idea is that PredatoryJournalRepository contains the data. ALL code uses that repo. - The repository is filled by the PredatoryJournalLoader. This is called by PredatoryJournalListMvGenerator. And NOWHERE else!

PredatoryJournalListMvGenerator is a new class with a main method similar to JournalListMvGenerator. Reason: Separation of concerns. Generating a journal abbrevation list is completely different from the predatory list generation. Thus, different main applications.

In other words:

  • PredatoryJournalRepository loads the .mv file.
  • PredatoryJournalLoader is only used ONCE in PredatoryJournalListMvGenerator. No other usages. Not in IntegrityChecks etc... They get their data from the PredatoryJournalRepository

Please hook PredatoryJournalListMvGenerator into the build process as described there:

tasks.register("generateJournalListMV", JavaExec) {

@Siedlerchr Siedlerchr requested a review from koppor November 12, 2023 16:21
* upstream/main: (24 commits)
  Clean up test code (JabRef#10658)
  Bump org.openrewrite.rewrite from 6.4.0 to 6.5.4 (JabRef#10650)
  Bump com.puppycrawl.tools:checkstyle from 10.12.4 to 10.12.5 (JabRef#10654)
  Bump com.fasterxml.jackson.dataformat:jackson-dataformat-yaml (JabRef#10653)
  Bump com.github.Dansoftowner:jSystemThemeDetector from 3.6 to 3.8
  Remove comments - don't work on forks
  Update journal abbreviation lists (JabRef#10645)
  Use clparse (instead of heylogs) (JabRef#10641)
  Update CSL styles (JabRef#10642)
  Added parent field to Hayagriva YAML export (JabRef#10633)
  Bump org.fxmisc.richtext:richtextfx from 0.11.1 to 0.11.2 (JabRef#10637)
  Bump io.github.classgraph:classgraph from 4.8.163 to 4.8.164 (JabRef#10634)
  Bump org.junit.platform:junit-platform-launcher from 1.10.0 to 1.10.1 (JabRef#10635)
  Bump org.mockito:mockito-core from 5.6.0 to 5.7.0 (JabRef#10638)
  Bump com.tngtech.archunit:archunit-junit5-engine from 1.1.0 to 1.2.0 (JabRef#10636)
  add some more spaces
  Fix CHANGELOG.md
  Remove SearchListener
  Refactor method name to reflect usage
  Revert "Remove filtering on query in maintable"
  ...
@Siedlerchr Siedlerchr enabled auto-merge December 4, 2023 20:12
Siedlerchr
Siedlerchr previously approved these changes Dec 4, 2023
koppor
koppor previously approved these changes Dec 4, 2023
@Siedlerchr Siedlerchr added this pull request to the merge queue Dec 4, 2023
@koppor koppor removed this pull request from the merge queue due to a manual request Dec 4, 2023
@Siedlerchr Siedlerchr enabled auto-merge December 4, 2023 20:15
@koppor koppor dismissed stale reviews from Siedlerchr and themself via c7991b6 December 4, 2023 20:28
@Siedlerchr Siedlerchr added this pull request to the merge queue Dec 5, 2023
Merged via the queue into JabRef:main with commit d03a43c Dec 5, 2023
@linsui
Copy link
Contributor

linsui commented Jan 18, 2024

Hi, when I update the jabref package for nixos I found that I can't generate the list in an offline sandbox. And it's hard to make the list reproducible because it fetches data from webpages. Any suggestion what I should do?

@koppor
Copy link
Member

koppor commented Jan 22, 2024

@linsui Are you a programmer or "just" a package creator?

If a programmer: We need to work together to modify the feature. See discussion at https://discourse.jabref.org/t/double-check-the-bibliography-for-predatory-publishers-and-journals/1738/7.

If a package creator: Maybe, it is possible to mock a HTTP server containing some data?

Maybe a quick fix can be to just return an empty list in case of a connection error?

@linsui
Copy link
Contributor

linsui commented Jan 23, 2024

Hi, I'm just a package maintainer and I don't know much about java programming. :)

Maybe a quick fix can be to just return an empty list in case of a connection error?

Good idea. :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Integrate check-bib-for-predatory
5 participants