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

Adding an advanced data structure for Title/Word in the CaseChangers … #215

Merged
merged 2 commits into from
Oct 9, 2015

Conversation

simonharrer
Copy link
Contributor

…implementation.

@koppor this is what I ment. What do you think?

@simonharrer simonharrer force-pushed the improve-case-changers branch from 150fdff to edf5f66 Compare October 8, 2015 10:44
simonharrer added a commit that referenced this pull request Oct 9, 2015
Adding an advanced data structure for Title/Word in the CaseChangers …
@simonharrer simonharrer merged commit 3a89147 into master Oct 9, 2015
@simonharrer simonharrer deleted the improve-case-changers branch October 9, 2015 09:11
@koppor
Copy link
Member

koppor commented Oct 9, 2015

While trying to make test cases, I discovered net.sf.jabref.bst.BibtexCaseChangersTest.testChangeCase(), which seems to cover most of our wishes and even can handle intra word casings correctly: Vall{\\'e}e -> VALL{\\'E}E. HAllo {worLD}. HOW -> hallo {worLD}. how.

I would suggest to remove CaseChangers completely and to use net.sf.jabref.bst.BibtexCaseChanger only to avoid code redundancy. I am aware that the code of BibtexCaseChanger is more ugly than the current code of CaseChangers, but it covers the strange cases (e.g., case also LaTeX characters).

@simonharrer
Copy link
Contributor Author

I will have a look. But the bst package is a candidate to be thrown away as
it is unmaintainable.

@simonharrer
Copy link
Contributor Author

I had a look: The CaseChangers have Title, Upper, Lower, UpperEachFirst and UpperEach whereas the BibtexCaseChangers only have Title, Upper and Lower. Small words are not detected in the BibtexCaseChangers algorithm. The only thing that the BibtexCaseChangers are better in is the detection of special latex characters like umlauts.

From my perspective, the BibtexCaseChangers class is hard to maintain. And it is used heavily in the bst package, making it very hard to improve the class as its interface has to stay the same. I think the current solution is good enough for a lot of use cases already. We would loose a lot and gain too little if we would replace the CaseChangers by the BibtexCaseChangers.

I would suggest to leave the BibtexCaseChangers class alone, and in the future remove the bst package and replace it with something else. Or just delete it. ;)

If someone has really a lot of time, they can use the test cases to drive an improved CaseChangers version. One simply has to determine a different boolean[] protectedChars array based on the title, namely, to not make every char within curly braces protected.

@koppor
Copy link
Member

koppor commented Oct 10, 2015

I made the interface a bit more modern (using enum instead of char as parameter). Still procedural style than object oriented, but for that use case, I think procedural style is more fitting. 😇

I made test cases based on #176 (comment) at

. Think, it's time for a call.

Siedlerchr added a commit that referenced this pull request Mar 14, 2021
79c4dba80a copied .github/workflows/merge.yaml .github/workflows/sheldon.yaml from styles
444eafb731 copied .github/workflows/sheldon.yaml from styles
810aad5bbc Add Hindi locale file (#216)
81e7a4db3e copied .github/workflows/sheldon.yaml from styles
01e105d03f copied .github/workflows/sheldon.yaml from styles
5e7a243493 copied .github/workflows/sheldon.yaml from styles
0cc2f75795 copied .github/workflows/sheldon.yaml from styles
01ccfd6e97 Update locales.json
5627bdaadb Update locales.json
af8f991570 Stop notifying 8827 port on Zotero servers (#215)
3ad32f0fb9 copied .github/workflows/merge.yaml from styles

git-subtree-dir: buildres/csl/csl-locales
git-subtree-split: 79c4dba80a16ad71a1ef462dcdba4db48e4f77ba
@Thomas-Bridgart Thomas-Bridgart mentioned this pull request Oct 27, 2024
7 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants