Implemented #1345 - cleanup ISSN

[oscargus]

Implemented cleanup that adds a missing dash in the ISSN field if it looks like an ISSN (checksum is not controlled, but eight digits, where the last may be an x).

• Change in CHANGELOG.md described
• Tests created for changes

[tobiasdiez]

I would move this functionality to a separate ISSN class which does the parsing. For example, the code then might look as follows:

ISSN issn = ISSN.fromString(issnText);
if (issn.isValid()) {
   entry.setField("issn", issn.ToString())
}

Then you could reuse the same logic to add a ISSN integrity check. The same remark applies to your other ISBN PR.

[oscargus]

How would you handle a string "3457345X"? Not set it? Add a dash? What about an arbitrary string?

Either way, I won't implement this at the moment.

[tobiasdiez]

I don't get your question, sorry. If the passed string is not matched by the regex, then issn.isValid returns false. So it is essentially just moving your code to three different methods. You may have a look at https://github.com/JabRef/jabref/blob/a6769dd81782a0010681fdd14622c1fcc43519fb/src/main/java/net/sf/jabref/logic/mods/PageNumbers.java

[oscargus]

So, what if it is a valid ISSN, but the dash is missing?

[Siedlerchr]

An ISSN would then not be valid!
It must contain an dash:
https://en.wikipedia.org/wiki/International_Standard_Serial_Number

[oscargus]

Sure, but it can be cleaned up as opposed to all other cases. Maybe I was focusing too much on your example, but to not set the field based on a missing dash seemed quite stupid to me. Better then to add it. Hence the questions how to handle that. Still, I do not really see it happening at the moment though.

[tobiasdiez]

So your problem is that for this PR you want to make sense of strings which look like ISSNs but are not quite in the standard format, right? What about creating static Optional<ISSN> fromString(text) which returns an ISSN object if JabRef could make sense of the input. Then the ISSN class has a constructor accepting the different parts of the ISSN (first part, second part and checkdigit), isValid returns true if the checksum is correct and toString formats it in the standard way using a separating dash. Would this be a solution?

[stefan-kolb]

Something similar should be in DOI.java if I remember correctly.

[oscargus]

The whole purpose of this PR was indeed to add the missing dash if it otherwise looks like an ISSN, yes. That works at the moment.

Would we actually need a clean up action then? If properly used, the ISSN class would provide automatic clean up, right? Which is useful, but may be annoying?

Btw, I doubt using the different parts explicitly is very useful though. But if there is a valid use case where the string is not enough, so sure. Should the parts then be String or int (x is 10 and only appears in the checksum)?

It really looks like over-engineering to me, I must say though...

[oscargus]

Extracted an ISSN class for checking validity and cleaning up.

[simonharrer]

I am missing tests for the ISSN class. Otherwise, LGTM.