-
-
Notifications
You must be signed in to change notification settings - Fork 2.6k
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
Implementation of new modifiers for issue #11367 #12067
base: main
Are you sure you want to change the base?
Conversation
CamelN Camel ShortTitleFormatter VeryShortTitleFormatter
CamelN Camel ShortTitleFormatter VeryShortTitleFormatter
# Conflicts: # src/main/java/org/jabref/logic/formatter/casechanger/CamelFormatter.java # src/main/java/org/jabref/logic/formatter/casechanger/CamelNFormatter.java # src/main/java/org/jabref/logic/formatter/casechanger/ShortTitleFormatter.java # src/main/java/org/jabref/logic/formatter/casechanger/VeryShortTitleFormatter.java
# Conflicts: # src/main/java/org/jabref/logic/formatter/casechanger/CamelFormatter.java # src/main/java/org/jabref/logic/formatter/casechanger/CamelNFormatter.java # src/main/java/org/jabref/logic/formatter/casechanger/ShortTitleFormatter.java # src/main/java/org/jabref/logic/formatter/casechanger/VeryShortTitleFormatter.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.
First impression: Looks good.
There should be tests for the use of veryhorttitle
etc. I think, you will find the existing tests and then you can refine them.
# Conflicts: # src/main/java/org/jabref/logic/formatter/casechanger/CamelFormatter.java # src/main/java/org/jabref/logic/formatter/casechanger/CamelNFormatter.java # src/main/java/org/jabref/logic/formatter/casechanger/ShortTitleFormatter.java # src/main/java/org/jabref/logic/formatter/casechanger/VeryShortTitleFormatter.java
@koppor |
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.
You can remove the tests, because the functionality of the called formatters are already tested (otherwise, I need to tell you: use @ParameterizedTest
, do not prefix tests with test_
and we would need another tough round here)
I am sorry that I wrote "I think, you will find the existing tests and then you can refine them.". Here is the pointer: Add before org.jabref.logic.citationkeypattern.BracketedPatternTest#expandBracketsWithAuthorStartingWithBrackets
- Add a new
@ParameterizedTest
with@CsvSource
. You see at other JabRef code how it works (use Ctrl+Shift+F). - Add tests for a variant of the cases mentioned at Special field markers and modifiers #11367 (comment). Use field
TITLE
instead ofUSER_CUSTOM_TITLE_FIELD
.
src/main/java/org/jabref/logic/formatter/casechanger/CamelFormatter.java
Outdated
Show resolved
Hide resolved
src/main/java/org/jabref/logic/formatter/casechanger/CamelNFormatter.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.
JUnit tests are failing. In the area "Some checks were not successful", locate "Tests / Unit tests (pull_request)" and click on "Details". This brings you to the test output.
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.
Describe the changes you have made here:
Implementation for issue: #11367
I have implemented new modifiers that can be used when renaming a PDF file used in file name format based on this comment: #11367 (comment) .
List of implemented modifiers:
Camel
CamelN
VeryShortTitle
ShortTitle
User documentation needs to be updated:
https://docs.jabref.org/setup/citationkeypatterns#modifiers
Mandatory checks
CHANGELOG.md
described in a way that is understandable for the average user (if applicable)