Remove %n in front and back of string by jmslbam · Pull Request #24 · adamlc/address-format

jmslbam · 2024-09-15T21:36:55Z

So if RECIPIENT or ORGANIZATION are not filled or empty, then this results in only a STREET_ADDRESS that's prefix with an %n which will be replace by a   which results in an address that's starts a line lower then needed.

Fixes #23

So if `RECIPIENT` or `ORGANIZATION` are not filled or empty, then this results in only a `STREET_ADDRESS` that's prefix with an `%n` which will be replace by a ` ` which results in an address that's starts a line lower then needed. Fixes #23

adamlc

Thanks for your PR!

It looks like the tests are now failing. Would you mind updating them please?

jmslbam · 2024-09-16T09:38:57Z

@adamlc, Ah yes I see that now. As it's not related to the scope of this issue, shall I update them in a different branch so those can be merged first before proceding with this PR's.

The following actions use a deprecated Node.js version and will be forced to run on node20: actions/checkout@v3, actions/cache@v3. For more info: https://github.blog/changelog/2024-03-07-github-actions-all-actions-will-run-on-node20-instead-of-node16-by-default/

Looks like we be needing actions/checkout@v4.

adamlc · 2024-09-17T10:26:36Z

I'll be honest I haven't used this library or php for years, but it looks like the unit test is actually broken.

1) FormatTest::testDeAddressFormat
Failed asserting that two strings are equal.
--- Expected
+++ Actual
@@ @@
 'Eberhard Wellhausen\n
 Wittekindshof\n
 Schulstrasse 4\n
-32547 Oyenhause'
+32547 Oyenhausen'

The final n is being trimmed off the address, which could mean that it is related to this change.

Otherwise you could end up with 2 `%n%n` after each other that get converted to ` `

So you don't end up with '%n%n' which gets converted to ` `

jmslbam · 2025-01-10T13:29:19Z

I'll be honest I haven't used this library or php for years, but it looks like the unit test is actually broken.
The final n is being trimmed off the address, which could mean that it is related to this change.

Sorry for the slow response, new dad here :)

You're right, I updated the PR and also added a test for this scenario 👍

adamlc · 2025-01-10T13:50:42Z

tests/FormatTest.php

+	$this->assertEquals(
+		$this->container->formatAddress(),
+		"Schulstrasse 4\n32547 Oyenhausen"
+	);


Just a small niggle to fix the indent, otherwise test looks good I think!

Like this? On 1 line? Because I copied it from the test about and just removed the Eberhard Wellhausen\nWittekindshof\n

@jmslbam sorry I meant indentation, you can see from the diff above it just needs bumping right

Hmmz, which line exactly? Because in the div/panel of the PR makes it so that the lines visually jump to a new line. If you just view the file by itself / raw I don't see any issues with the new Code/Test

Can you screenshot / point which line it is?

https://github.com/adamlc/address-format/blob/b042c814a366d76e68615c57f706886048acd779/tests/FormatTest.php

To much indention

What I do see it that the other test have an extra indention which should be there from any kind of coding standard. I added a screenshot.

Thank you for taking the time and I can fix the overal indention in a new PR, no problemo

I'm just looking in my browser at the github pull request

Ok I changed the tabs to spaces, will fix the other spacing / indention issues in a different PR and will add a .editorconfig to the repo which defines the settings for this project (spaces).

The FormatTest.php file mixes tabs and spaces as indentions.

Or if you ok, I can fix it in this same PR, save you time.

Feel free to add it into this PR. Greatly appreciated @jmslbam. The issues must have slipped through the net over the years and I've not noticed!

adamlc · 2025-01-14T08:28:48Z

Amazing thanks for all your work @jmslbam

jmslbam · 2025-01-14T12:59:44Z

@adamlc you're welcome! Thank you for the initial work of the package!

Remove %n in front and back of string

0ad0c77

So if `RECIPIENT` or `ORGANIZATION` are not filled or empty, then this results in only a `STREET_ADDRESS` that's prefix with an `%n` which will be replace by a ` ` which results in an address that's starts a line lower then needed. Fixes #23

adamlc approved these changes Sep 16, 2024

View reviewed changes

adamlc requested changes Sep 16, 2024

View reviewed changes

jmslbam added 2 commits January 10, 2025 14:24

Remove unused %n when input_map[$value] is empty

79ec62e

Otherwise you could end up with 2 `%n%n` after each other that get converted to ` `

Update test when keeping attributes empty

2410afe

So you don't end up with '%n%n' which gets converted to ` `

jmslbam requested a review from adamlc January 10, 2025 13:28

adamlc requested changes Jan 10, 2025

View reviewed changes

Set assertEquals to 1 line

b042c81

jmslbam requested a review from adamlc January 10, 2025 23:11

jmslbam added 2 commits January 13, 2025 15:32

Changer from tabs to spaces

3e1c4d4

Add .editorconfig and convert tabs to spaces

665cf1e

adamlc approved these changes Jan 14, 2025

View reviewed changes

adamlc merged commit b7071fe into adamlc:master Jan 14, 2025
1 check passed

adamlc mentioned this pull request Jan 22, 2025

Leftover %S for addresses #26

Closed

iMusicJJ mentioned this pull request Jan 22, 2025

Fix hanging markers #27

Merged

Conversation

jmslbam commented Sep 15, 2024

Uh oh!

adamlc left a comment

Choose a reason for hiding this comment

Uh oh!

jmslbam commented Sep 16, 2024

Uh oh!

adamlc commented Sep 17, 2024

Uh oh!

jmslbam commented Jan 10, 2025

Uh oh!

adamlc Jan 10, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jmslbam Jan 10, 2025

Choose a reason for hiding this comment

Uh oh!

adamlc Jan 13, 2025

Choose a reason for hiding this comment

Uh oh!

jmslbam Jan 13, 2025

Choose a reason for hiding this comment

To much indention

Uh oh!

adamlc Jan 13, 2025

Choose a reason for hiding this comment

Uh oh!

jmslbam Jan 13, 2025

Choose a reason for hiding this comment

Uh oh!

adamlc Jan 13, 2025

Choose a reason for hiding this comment

Uh oh!

jmslbam Jan 13, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

adamlc commented Jan 14, 2025

Uh oh!

jmslbam commented Jan 14, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

adamlc Jan 10, 2025 •

edited

Loading