Do not trim or remove whitespace from the title based id fetcher #4016
Conversation
| fetcher = WebFetchers.getIdBasedFetchers(Globals.prefs.getImportFormatPreferences()).get(comboBox.getSelectedIndex()); | ||
|
|
||
| searchID = idTextField.getText(); | ||
| if (!(fetcher instanceof TitleFetcher)) { |
There was a problem hiding this comment.
Why do we remove whitespace for the other fetcher? Shouldn't each fetcher decide how to proceed the input? I would just always trim it at this point...
There was a problem hiding this comment.
All other fetchers have a single alphanumeric id or an url so, trimming in general makes sense.
There was a problem hiding this comment.
Copy and paste DOIs from PDFs or homepages (e.g., Springer) leads to identifiers. With the fix #3697, there is a speedup in the user workflow. One can just copy something from a PDF/homepage and paste it into JabRef and click "Generate".
From JabRef#283:
http s://doi.org/10.1007/s13222-018-0273-1 and DOI: 10.1109/MS.2013.97.
There was a problem hiding this comment.
I don't say we should remove the functionality. It makes sense and indeed streamlines the user flow. I was arguing in favor of placing this code .trim().placeAll(" ", "") in every fetcher that supports it. It terms of coding principles, the current solution violates the "hide implementation details" principle, since the caller of the interface needs to know which fetcher is invoked (TitleFetcher or not) and whether its support white space removal. Moreover, I bet that if somebody implements a new fetcher in a year that relies on correct white space, then he will forget to add this new fetcher to this exception list here.
There was a problem hiding this comment.
I think it really makes sense to add this argument parsing/trimming to the fetcher itself as @tobiasdiez proposes. Maybe a new method parse input?
…itespace * upstream/master: Fix casing in java file name. (#4029)
add some tests
|
I now moved trimming down to the concrete fetcher classes and added tests |
| @@ -44,7 +44,9 @@ public HelpFile getHelpPage() { | |||
|
|
|||
| @Override | |||
| public Optional<BibEntry> performSearchById(String identifier) throws FetcherException { | |||
| Optional<DOI> doi = DOI.parse(identifier); | |||
| String cleanedIdentifier = identifier.trim(); | |||
There was a problem hiding this comment.
Can you please move this clean-up to the DOI.parse method.
add test
Fixes #4014