-
-
Notifications
You must be signed in to change notification settings - Fork 2.8k
Add support for LTWA (issue #12276) #12880
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
Conversation
src/main/java/org/jabref/logic/journals/JournalAbbreviationLoader.java
Outdated
Show resolved
Hide resolved
src/main/java/org/jabref/logic/journals/JournalAbbreviationRepository.java
Show resolved
Hide resolved
src/main/java/org/jabref/logic/journals/JournalAbbreviationRepository.java
Outdated
Show resolved
Hide resolved
src/main/java/org/jabref/logic/journals/ltwa/LtwaRepository.java
Outdated
Show resolved
Hide resolved
src/test/java/org/jabref/logic/journals/LtwaRepositoryTest.java
Outdated
Show resolved
Hide resolved
try (InputStream resourceAsStream = JournalAbbreviationRepository.class.getResourceAsStream("/journals/ltwa-list.mv")) { | ||
if (resourceAsStream == null) { | ||
LOGGER.warn("There is no ltwa-list.mv. We cannot load the LTWA repository."); | ||
return null; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Optional?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I changed it to make it throw instead of null and let the public method take care of that.
I have updated the implementation to use Antlr. I am not sure if this issue is reproducible: whenever I tried to use the Antlr-generated source to develop on my local machine after the initial |
src/main/java/org/jabref/logic/journals/ltwa/LtwaRepository.java
Outdated
Show resolved
Hide resolved
src/main/java/org/jabref/logic/journals/ltwa/NormalizeUtils.java
Outdated
Show resolved
Hide resolved
src/main/java/org/jabref/logic/journals/ltwa/NormalizeUtils.java
Outdated
Show resolved
Hide resolved
src/test/java/org/jabref/logic/journals/LtwaRepositoryTest.java
Outdated
Show resolved
Hide resolved
src/main/java/org/jabref/logic/journals/ltwa/LtwaRepository.java
Outdated
Show resolved
Hide resolved
src/main/java/org/jabref/logic/journals/ltwa/NormalizeUtils.java
Outdated
Show resolved
Hide resolved
…r-issue-12273 # Conflicts: # src/main/java/org/jabref/cli/LtwaListMvGenerator.java # src/main/java/org/jabref/gui/journals/UndoableAbbreviator.java # src/main/java/org/jabref/logic/journals/ltwa/LtwaRepository.java # src/main/java/org/jabref/logic/journals/ltwa/LtwaTsvParser.java # src/main/java/org/jabref/logic/journals/ltwa/NormalizeUtils.java # src/test/java/org/jabref/logic/journals/LtwaRepositoryTest.java
} | ||
|
||
SearchState state = new SearchState(node, index); | ||
if (visited.contains(state)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm unsure where I nest the logic inside the "else" branches; this method is recursive.
try (InputStream resourceAsStream = JournalAbbreviationRepository.class.getResourceAsStream("/journals/ltwa-list.mv")) { | ||
if (resourceAsStream == null) { | ||
LOGGER.warn("There is no ltwa-list.mv. We cannot load the LTWA repository."); | ||
return null; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I changed it to make it throw instead of null and let the public method take care of that.
var languageStr = matcher.group(3); | ||
|
||
word = NormalizeUtils.normalize(ANNOTATION.matcher(word).replaceAll("").strip()); | ||
var abbreviation = abbreviationStr.equals("n.a.") ? null : abbreviationStr; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would like to acknowledge that it's not possible to serialize Optional
, and I figured it would be unwise to store an extra field called isAbbreviationPresent
and store something arbitary in the actual abbreviation. As such, I left it null.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Impressive! Works
@trag-bot didn't find any issues in the code! ✅✨ |
Integrate journal abbreviation toggle functionality (JabRef#12880) with the LTWA repository support from main branch. Resolve conflicts in JournalAbbreviationRepository, JournalAbbreviationLoader, and MainMenu to ensure both features work correctly together. The combined functionality allows users to enable/disable specific journal abbreviation sources while maintaining LTWA abbreviation support.
* Implement LTWA abbreviation * Create LTWA resource download in the gradle tasks * Connected the LTWA logic with the GUI * Updated the CHANGELOG.md * Fix according to Trag * Reimplement with Antlr * Fix errors from previous PR * Use Optional as Trag suggested * Fix the locale translation issue --------- Co-authored-by: Christoph <[email protected]>
* Implement LTWA abbreviation * Create LTWA resource download in the gradle tasks * Connected the LTWA logic with the GUI * Updated the CHANGELOG.md * Fix according to Trag * Reimplement with Antlr * Fix errors from previous PR * Use Optional as Trag suggested * Fix the locale translation issue --------- Co-authored-by: Christoph <[email protected]>
Closes #12273
src/main/java/org/jabref/gui/journals
to handle the UI logics.LTWARepository
class and integrated it as a part of theJournalAbbreviationRepository
to handle the LTWA abbreviation logics. I am not exactly sure if it would be appropriate to change the abbreviation class since that class appears to be largely a data class, and I am hesitant to add business logics to it.Mandatory checks
CHANGELOG.md
described in a way that is understandable for the average user (if change is visible to the user)