-
-
Notifications
You must be signed in to change notification settings - Fork 3k
add: CAYW endpoint formats #13785
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
add: CAYW endpoint formats #13785
Conversation
59f035c
to
cf113a2
Compare
jabsrv/src/test/java/org/jabref/http/server/cayw/CAYWResourceTest.java
Outdated
Show resolved
Hide resolved
@palukku Hi Philip, I made a mess in commit history on my remote branch, so asked AI what to do. At the end, the commit history looks good now, I pushed with force and the test failed. What can I do to resolve this? |
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.
Is there a specific reason on why you switched from the map and registry approach to the new switch/case approach?
I'm fine with both but I'm curious what your thoughts behind this were
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.
latex
, cite
, citep
and citet
are alias of natbib
, so they share the same output format. However, if command
is not specified, the default value of each format is:
Format | Default command |
---|---|
latex cite natbib |
cite |
citep |
citep |
citet |
citet |
At first, I tried to keep the registry approach by doing below in, for example, NatbibFormatter
:
// Replace CAYWFormatter#getFormatName by below method
@Override
public List<String> getFormatNames() {
return List.of("natbib", "latex", "cite", "citep", "citet");
}
@Override
public String format(CAYWQueryParams queryParams, List<CAYWEntry> caywEntries) {
String command = queryParams.getCommand().orElseGet(() -> {
String format = queryParams.getFormat();
switch (format) {
case "natbib":
case "latex":
case "cite":
return "cite";
case "citep":
return "citep";
case "citet":
return "citet";
default:
return "cite";
}
});
...
}
In this way, I have 2 concerns:
queryParams.getFormat()
in theory will not be null, but I can't 100% feel safe about this. Just in case it is null, I don't have a nice way of handling it.- If we have a new format using the same Formatter, then we need to add the new format in both
getFormatNames
andformat
.
Because of above, I put the the new switch/case approach in the FormatterService.java class. I am happy to edit if this is not a good approach. Thanks.
jabsrv/src/test/java/org/jabref/http/server/cayw/format/CAYWFormattersTest.java
Outdated
Show resolved
Hide resolved
jabsrv/src/test/java/org/jabref/http/server/cayw/format/CAYWFormattersTest.java
Outdated
Show resolved
Hide resolved
No need to worry, next time just add new commits it does not matter if you have changes later on on things you also have in an earlier commit because as the bot already wrote will get squashed anyway. |
Currently I have very limited time, I will do a full review on thursday evening. |
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/FormatterService.java
Outdated
Show resolved
Hide resolved
.toList(); | ||
|
||
return bibEntries.stream() | ||
.map(entry -> entry.getCitationKey().map("@%s"::formatted)) |
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.
Maybe we should use the full cite command (example there: #cite(<arrgh>)
) also think about:
If your source name contains certain characters such as slashes, which are not recognized by the <> syntax, you can explicitly call label instead.
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.
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 it to only add the label thing when there is a slash present, so it is only used when necessary.
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 modified such that when there are multiple keys, only the ones containing slash will be formatted with the label thing, and the other keys will use the @
thing. Please see whether this is what you intend. Thanks.
void mmd() { | ||
MMDFormatter formatter = new MMDFormatter(); | ||
String actual = formatter.format(queryParams(null), caywEntries("key1", "key2")); | ||
// Whatever your MMD formatter currently emits; adjust expected accordingly. |
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 comment and the othere smells like ai? Also please remove, adds nothing meaningful to the code.
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.
Yes, I did use AI to generate the tests, then I checked the code. Sorry that I overlooked this.
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.
For this it's okay, but don't rely too much on ai when coding and always double check what the ai suggests.
Also delete the comments here: https://github.com/JabRef/jabref/pull/13785/files#diff-d3db9dc636031d931c6fe5cdfe69aff341619cda38fa586964c3b3bf6bc1d674R53-R56
…improve code readability of switch statement.
jabsrv/src/test/java/org/jabref/http/server/cayw/format/CAYWFormattersTest.java
Show resolved
Hide resolved
jabsrv/src/main/java/org/jabref/http/server/cayw/format/TypstFormatter.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.
Looks good to me
jabsrv/src/test/java/org/jabref/http/server/cayw/format/CAYWFormattersTest.java
Outdated
Show resolved
Hide resolved
…rmattersTest.java
jabsrv/src/test/java/org/jabref/http/server/cayw/format/CAYWFormattersTest.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.
Now it should work and looks good
jabsrv/src/test/java/org/jabref/http/server/cayw/format/CAYWFormattersTest.java
Outdated
Show resolved
Hide resolved
…rmattersTest.java
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 should avoid committing things on my phone next time
@trag-bot didn't find any issues in the code! ✅✨ |
* upstream/main: (54 commits) Split relativizeSymlinks parameterized tests in separate tests (#13782) Update the search syntax highlight for web search (#13801) Chore(deps): Bump ai.djl:bom from 0.33.0 to 0.34.0 in /versions (#13833) Fix typos in CHANGELOG.md (#13826) Chore(deps): Bump com.konghq:unirest-modules-gson in /versions (#13831) Chore(deps): Bump org.gradlex:extra-java-module-info in /build-logic (#13830) Chore(deps): Bump org.apache.logging.log4j:log4j-to-slf4j in /versions (#13832) Chore(deps): Bump io.zonky.test.postgres:embedded-postgres-binaries-bom (#13834) Chore(deps): Bump jablib/src/main/resources/csl-locales (#13829) Chore(deps): Bump jablib/src/main/resources/csl-styles (#13827) Chore(deps): Bump jablib/src/main/abbrv.jabref.org (#13828) add: CAYW endpoint formats (#13785) New Crowdin updates (#13823) chore(deps): update dependency org.kohsuke:github-api to v2.0-rc.5 (#13822) Add support for automatic ICORE conference ranking lookup [#13476] (#13699) New Crowdin updates (#13820) Initialize search bar auto-completion with real database context (no tab switch needed) (#13816) Fixes #13274: Allow cygwin-paths on Windows (#13297) Refine "REDACTED" replacement of API key value in web fetcher search URL (#13814) changed ISSNCleanup into NormalizeIssn, refactored respective tests #13748 (#13767) ...
Closes #13578
Added formatters mentioned in the issue and http tests on
org.jabref.http.server.CAYWResource
Steps to test
Follow the guideline on https://devdocs.jabref.org/code-howtos/http-server.html#start-http-server to start the server.
Examples of command to run are:
Mandatory checks
CHANGELOG.md
in a way that is understandable for the average user (if change is visible to the user)