Fix hanging markers

[iMusicJJ]

This fixes the issue introduced in #24, discussed in #26 with hanging %S markers.

A failing test was added, and subsequently fixed

[iMusicJJ]

I am not entirely sure the %n was added to the replacement key - all tests seem to pass without it.

[jmslbam]

Hi @iMusicJJ,

This revert and fixes the current test.

But please take in account that when you leave out for example the postalcode and write a test for that it will fail with this code.

This would fail because the code would return Girona and not Girona.

$this->container->setAttribute('LOCALITY', 'Girona');
$this->container->setAttribute('RECIPIENT', 'Jesper Jacobsen');
$this->container->setAttribute('POSTAL_CODE', '');
$this->container->setAttribute('STREET_ADDRESS', 'Gran Via De Jaume X, 123');

$this->assertEquals(
    "Jesper Jacobsen\nGran Via De Jaume X, 123\nGirona",
    $this->container->formatAddress()
);

Therefor the #23 was noticed and the current 2.0 version was introduced. Leaving some scenarions unfixed. I will pick this up and fix it and extend the test some more.

[jmslbam]

Oh and sorry, thanks for noticing and opening the PR and issue, efforts are much appreciated! 🙌

[iMusicJJ]

Alright. Can you add a failing test case please?

Makes testing that there is no regression easier. It wasn't clear to me why the code was changed, as there was no failing tests.

[iMusicJJ]

This would fail because the code would return Girona and not Girona.

A trim in the correct spot (around each individual part, should be able to fix this, right?

Alas, if time is short, I'd like to continue the PR, so we can get the issue fixed.

This revert and fixes the current test.

Where does it fix the current test? I don't think I am touching any tests, except adding a new one?

[iMusicJJ]

@jmslbam I have now added your failing test-case, and added a fix + confirmed it via the failing test.

@adamlc Feel free to give this a once-over.

It should be a fair bit more robust now in not leaving markers + spaces hanging.

Once the fully formatted address is generated. It is dissected and trimmed on a line-by-line basis, and finally reassembled from the trimmed parts. Lastly all double-or-more spaces are replaced with a single space.

[adamlc]

@iMusicJJ thanks for your updates.

I just ran the unit tests and it looks like they're failing with an invalid locale error. At a guess I would say it needs to be ES rather than es. The linux runner will be case sensitive.

Otherwise I think the changes look good and will be hopefully more robust!

[iMusicJJ]

Alright, I'll take a look at the pipeline and see

[iMusicJJ]

Hey @adamlc, it should be fixed now.

The issue was a case-sensitive filesystem, which my dev machine isn't.

I've allowed both lowercase and uppercase locales now, so both ->setLocale('es') and ->setLocale('ES') works.
Before only ->setLocale('ES') worked, the other one would throw an error.

[adamlc]

Good work on the locale parsing. Although it looks like its now broken another test as the behaviour has changed 🤔

[iMusicJJ]

Still seems to fail. I'll take a look.

Edit: just an outdated test, that seems to test that lowercase locales were not allowed. I'll change it to test a non existent locale instead

[iMusicJJ]

Nvm, there was a Test.json test-file, which didn't follow the syntax of being uppercase. (Thus throwing a not-found error, instead of invalid parsing error)

Can you try again?

[iMusicJJ]

I have moved Test.json to _INVALID.json to move it out of the normal ISO-LOCALE namespace, and clarify that it is special.

[adamlc]

Looks great thanks, will create a release now

[iMusicJJ]

Thanks, and thank you for the swift responses!