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

Fix Abbreviation for Escaped Ampersand #9288

Merged
merged 26 commits into from
Nov 2, 2022
Merged
Show file tree
Hide file tree
Changes from 11 commits
Commits
Show all changes
26 commits
Select commit Hold shift + click to select a range
ed40cc2
Added Naive Ampersand Formatter, see last point in #8948 to-do
AkshatJain9 Oct 14, 2022
3dc87ae
Rudimentary attempt at allow \& in the journal abbreviation. However,…
ANUu7312578 Oct 15, 2022
e56af9e
Remove unnecessary code in BibWriter.java and unnecessary replacement…
ANUu7312578 Oct 20, 2022
ff15343
Merge branch 'main' of https://github.com/JabRef/jabref
ANUu7312578 Oct 20, 2022
bc31f93
Merge branch 'main' of https://github.com/AkshatJain9/jabref into iss…
ANUu7312578 Oct 20, 2022
4479e19
Merge branch 'JabRef:main' into main
AkshatJain9 Oct 22, 2022
03a996b
Merge branch 'JabRef:main' into issue-8948
AkshatJain9 Oct 22, 2022
1beef73
Merge branch 'main' of https://github.com/AkshatJain9/jabref into iss…
ANUu7312578 Oct 22, 2022
26b6527
Remove linebreak for checkstyle
ANUu7312578 Oct 22, 2022
97fdb55
Updated CHANGELOG.md to reflect the changes made
ANUu7312578 Oct 22, 2022
d34b5cd
Create tests for escaped ampersand in abbreviation
ANUu7312578 Oct 23, 2022
d8d0850
Merge branch 'main' of https://github.com/JabRef/jabref
ANUu7312578 Oct 25, 2022
64cc386
Merge branch 'main' of https://github.com/AkshatJain9/jabref into iss…
ANUu7312578 Oct 25, 2022
5426806
Remove unnecessary comments
ANUu7312578 Oct 25, 2022
0ed991b
Convert to using getResolvedFieldOrAliasLatexFree instead of replace()
ANUu7312578 Oct 26, 2022
d8a69c8
Added a javadoc to explain the function created
ANUu7312578 Oct 26, 2022
0efdc5a
Merge branch 'main' of https://github.com/JabRef/jabref
ANUu7312578 Oct 26, 2022
18d7d8b
Merge branch 'main' of https://github.com/AkshatJain9/jabref into iss…
ANUu7312578 Oct 26, 2022
e18c792
Added 3 tests to BibEntryWriterTest which test abbreviating journal e…
ANUu7312578 Oct 28, 2022
f4658bc
Merge branch 'main' of https://github.com/JabRef/jabref
ANUu7312578 Oct 28, 2022
943fa5b
Merge branch 'main' of https://github.com/AkshatJain9/jabref into iss…
ANUu7312578 Oct 28, 2022
7815ba0
Move journal abbreviation tests from BibEntryWriterTest to JournalAbb…
ANUu7312578 Oct 28, 2022
fc0907b
CHANGELOG.md changed to remove unnecessary information. restrictUsage…
ANUu7312578 Oct 29, 2022
0657b7e
Merge branch 'main' of https://github.com/JabRef/jabref
ANUu7312578 Oct 29, 2022
046498b
Merge branch 'main' of https://github.com/AkshatJain9/jabref into iss…
ANUu7312578 Oct 29, 2022
098bdac
Merge branch 'main' into issue-8948
Siedlerchr Nov 1, 2022
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -72,6 +72,7 @@ Note that this project **does not** adhere to [Semantic Versioning](http://semve
- We fixed an issue where hitting enter on the search field within the preferences dialog closed the dialog. [koppor#630](https://github.com/koppor/jabref/issues/630)
- We fixed a typo within a connection error message. [koppor#625](https://github.com/koppor/jabref/issues/625)
- We fixed an issue where the 'close dialog' key binding was not closing the Preferences dialog. [#8888](https://github.com/jabref/jabref/issues/8888)
- We fixed an issue where journal abbreviations would not abbreviate journal titles with escaped ampersands (\\&) which are written in accordance with Latex format. [#8948](https://github.com/JabRef/jabref/issues/8948)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would remove the last part, that information should be made available in the internet elsewhere 😃

Suggested change
- We fixed an issue where journal abbreviations would not abbreviate journal titles with escaped ampersands (\\&) which are written in accordance with Latex format. [#8948](https://github.com/JabRef/jabref/issues/8948)
- We fixed an issue where journal abbreviations would not abbreviate journal titles with escaped ampersands (\\&). [#8948](https://github.com/JabRef/jabref/issues/8948)


### Removed

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@
import java.util.Objects;
import java.util.Optional;
import java.util.Set;
import java.util.regex.Matcher;
import java.util.stream.Collectors;

import org.h2.mvstore.MVMap;
Expand Down Expand Up @@ -48,7 +49,8 @@ private static boolean isMatchedAbbreviated(String name, Abbreviation abbreviati
* Letters) or its abbreviated form (e.g. Phys. Rev. Lett.).
*/
public boolean isKnownName(String journalName) {
String journal = journalName.trim();
// Trims the String and also replaces any instances of "\&" with "&" for abbreviation search purposes
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Trims the String and also replaces any instances of "\&" with "&" is a literal explanation of the code and can be removed.

Suggested change
// Trims the String and also replaces any instances of "\&" with "&" for abbreviation search purposes
// The journal lists contains unescaped only, thus "\&" is replaced by "&" for abbreviation search purposes

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removed!

String journal = journalName.trim().replaceAll(Matcher.quoteReplacement("\\&"), "&");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think, there is the possibility to pre-compile the Regex. Was it with the Pattern class?


boolean isKnown = customAbbreviations.stream().anyMatch(abbreviation -> isMatched(journal, abbreviation));
if (isKnown) {
Expand All @@ -75,7 +77,8 @@ public boolean isAbbreviatedName(String journalName) {
* @param input The journal name (either abbreviated or full name).
*/
public Optional<Abbreviation> get(String input) {
String journal = input.trim();
// Trims the String and also replaces any instances of "\&" with "&" for abbreviation search purposes
String journal = input.trim().replaceAll(Matcher.quoteReplacement("\\&"), "&");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See above 😇


Optional<Abbreviation> customAbbreviation = customAbbreviations.stream()
.filter(abbreviation -> isMatched(journal, abbreviation))
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -134,4 +134,16 @@ void getFromFullName() {
void getFromAbbreviatedName() {
assertEquals(new Abbreviation("American Journal of Public Health", "Am. J. Public Health"), repository.get("Am. J. Public Health").get());
}

@Test
void testAbbreviationsWithEscapedAmpersand() {
assertEquals(new Abbreviation("ACS Applied Materials & Interfaces", "ACS Appl. Mater. Interfaces"), repository.get("ACS Applied Materials & Interfaces").get());
assertEquals(new Abbreviation("ACS Applied Materials & Interfaces", "ACS Appl. Mater. Interfaces"), repository.get("ACS Applied Materials \\& Interfaces").get());
assertEquals(new Abbreviation("Antioxidants & Redox Signaling", "Antioxid. Redox Signaling"), repository.get("Antioxidants & Redox Signaling").get());
assertEquals(new Abbreviation("Antioxidants & Redox Signaling", "Antioxid. Redox Signaling"), repository.get("Antioxidants \\& Redox Signaling").get());

repository.addCustomAbbreviation(new Abbreviation("Long & Name", "L. N.", "LN"));
assertEquals(new Abbreviation("Long & Name", "L. N.", "LN"), repository.get("Long & Name").get());
assertEquals(new Abbreviation("Long & Name", "L. N.", "LN"), repository.get("Long \\& Name").get());
Comment on lines +152 to +159
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Normally, we expect one assertation per test case. Thus, this method should be split into several ones. @ParamterizedTest is the the thign to use.

}
}