-
-
Notifications
You must be signed in to change notification settings - Fork 2.9k
Added CiteFormatter and BibLatexFormatter for CAYW feature #13704
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
jabsrv/src/main/java/org/jabref/http/server/cayw/format/BibLatexFormatter.java
Outdated
Show resolved
Hide resolved
jabsrv/src/main/java/org/jabref/http/server/cayw/format/CAYWFormatter.java
Outdated
Show resolved
Hide resolved
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.
AI?
PR description? steps to test? |
Why is this not a draft pr if the build is failing? |
Sorry it was mistake. I will be redoing this .Sorry for the inconvenience. |
public String format(CAYWQueryParams queryParams, List<CAYWEntry> cawEntries) { | ||
String command = queryParams.getCommand(); | ||
if (command == null || command.isBlank()) { | ||
command = "citep"; // default for citep |
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.
The comment 'default for citep' is redundant and does not provide any additional information beyond what is clearly visible in the code itself.
Your code currently does not meet JabRef's code guidelines. We use Checkstyle to identify issues. You can see which checks are failing by locating the box "Some checks were not successful" on the pull request page. To see the test output, locate "Source Code Tests / Checkstyle (pull_request)" and click on it. In case of issues with the import order, double check that you activated Auto Import. You can trigger fixing imports by pressing Ctrl+Alt+O to trigger Optimize Imports. Please carefully follow the setup guide for the codestyle. Afterwards, please run checkstyle locally and fix the issues, commit, and push. |
No need for re-do. Just edit PR description and push new commits. You can also change to draft at GitHub's UI. |
Note that your PR will not be reviewed/accepted until you have gone through the mandatory checks in the description and marked each of them them exactly in the format of |
JUnit tests of You can then run these tests in IntelliJ to reproduce the failing tests locally. We offer a quick test running howto in the section Final build system checks in our setup guide. |
.toList(); | ||
return "\\%s{%s}".formatted(command, | ||
bibEntries.stream() | ||
.map(entry -> entry.getCitationKey().orElse("")) |
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.
Empty string is used as a fallback for missing citation keys, which could lead to invalid LaTeX syntax. Should handle this case more explicitly or throw an exception.
List<BibEntry> bibEntries = caywEntries.stream() | ||
.map(CAYWEntry::bibEntry) | ||
.toList(); |
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.
This intermediate list creation is unnecessary since the stream is used immediately after. The operations can be chained directly for better performance.
// Handle "parencite" command with square brackets | ||
if ("parencite".equalsIgnoreCase(command)) { |
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.
The comment is trivial and only restates what is obvious from the code itself. Comments should add new information or reasoning that cannot be derived directly from the code.
.map(bibEntry -> "[@" + bibEntry.getCitationKey().orElse("") + "]") | ||
.collect(Collectors.joining("")); | ||
} else { | ||
// Default: bare @key format with semicolon separator |
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.
The comment merely describes what the code does without providing additional context or reasoning. Such trivial comments should be removed as they don't add value beyond what's visible in the code.
List<BibEntry> bibEntries = caywEntries.stream() | ||
.map(CAYWEntry::bibEntry) | ||
.toList(); |
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.
Intermediate list creation is unnecessary since the stream is used immediately after. The stream operations could be chained directly for better performance.
void testFormat() { | ||
CitepFormatter f = new CitepFormatter(); | ||
|
||
CAYWQueryParams qp = mock(CAYWQueryParams.class); // command is ignored |
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.
The comment provides no additional information beyond what is obvious from the code itself. Comments should add new information or reasoning, not restate what's visible.
void testFormat() { | ||
CitepFormatter f = new CitepFormatter(); | ||
|
||
CAYWQueryParams qp = mock(CAYWQueryParams.class); // command is ignored |
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.
Variable name 'qp' is too short and doesn't convey meaning. Variable names should be descriptive and include intention in their naming.
@Test | ||
@DisplayName("getFormatNames() returns the two expected aliases in order") | ||
void testGetFormatNames() { | ||
CitepFormatter f = new CitepFormatter(); |
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.
Variable name 'f' is too short and doesn't convey meaning. Variable names should be descriptive and include intention in their naming.
@Test | ||
void testGetFormatNamesNotEmpty() { | ||
LatexFormatter f = new LatexFormatter(); | ||
assertFalse(f.getFormatNames().isEmpty(), "Format-name list must not be empty"); |
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.
Using assertFalse to check collection emptiness is less specific than directly asserting the contents. Use assertions that provide better failure messages and more precise validation.
|
||
@Test | ||
void testHasAtLeastTwoAliases() { | ||
assertTrue(new NatbibFormatter().getFormatNames().size() >= 2, |
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.
Using assertTrue for size comparison is less descriptive than assertThat or assertEquals. The test should assert the actual contents of the collection rather than a boolean condition.
|
||
@Test | ||
void testGetFormatNames() { | ||
PandocFormatter f = new PandocFormatter(); |
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.
Variable name 'f' is too short and not descriptive. It should be named to reflect its purpose, such as 'formatter' or 'pandocFormatter'.
void testDefaultFormat() { | ||
PandocFormatter f = new PandocFormatter(); | ||
|
||
CAYWQueryParams qp = mock(CAYWQueryParams.class); |
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.
Variable name 'qp' is too short and not descriptive. It should be named to reflect its purpose, such as 'queryParams' or 'formatQueryParams'.
|
||
|
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.
Please set up your workspace with autoformatting and checkstyle according to our guide
https://devdocs.jabref.org/getting-into-the-code/guidelines-for-setting-up-a-local-workspace/intellij-13-code-style.html
|
||
List<BibEntry> bibEntries = caywEntries.stream() | ||
.map(CAYWEntry::bibEntry) | ||
.toList(); | ||
|
||
.map(CAYWEntry::bibEntry) | ||
.toList(); |
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.
checkstyle
avoid removing newlines introduced to separate different thoughts
@Test | ||
@DisplayName("parencite command ⇒ '[@k1][@k2]…'") | ||
void testParenciteFormat() { | ||
PandocFormatter f = new PandocFormatter(); |
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.
do not use abbreviations
BibEntry bib = mock(BibEntry.class); | ||
when(bib.getCitationKey()).thenReturn(Optional.of(key)); | ||
|
||
CAYWEntry cayw = mock(CAYWEntry.class); | ||
when(cayw.bibEntry()).thenReturn(bib); | ||
return cayw; |
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.
From what can be gathered from your code, the impression is that you do not know what these formats are, what JabRef is or even what a Bibtex entry is.
The code is heavily AI-generated.
Furthermore, due to insincerity in marking the mandatory checks:
(None of the commits so far have a passing build check - so I don't know how you manually tested)
I am inclined to close the PR.
Please gather some context about the project you are contributing to, read the devdocs on setting up a local workspace, look at some existing tests and maybe then take up an issue.
You don't seem to have any idea and just rely on AI. Thus closing |
Closes #13578
This PR extends the Cite-As-You-Write (CAYW) feature in JabRef to fully comply with the formats supported by Better BibTeX.
New supported formats:
natbib
latex
citep
mmd
pandoc
typst
Steps to test
Start JabRef with the server enabled: "./gradlew clean run"
Run the tests : "./gradlew :jabsrv:test"
verify that the following formats are now supported in addition to the existing ones:
natbib → should return \citep{key} style output
latex → should return \cite{key} style output
citep → explicitly handled citation with \citep{key}
mmd (MultiMarkdown) → should return [#key] style citations
pandoc → should return [key] style citations
typst → should return [key] style citations
Confirm all tests pass and JabRef doesn’t throw any errors when these formats are requested
Mandatory checks
CHANGELOG.md
described in a way that is understandable for the average user (if change is visible to the user)