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.

Sign up for GitHub

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

Jump to bottom

Remove %n in front and back of string #24

Merged
merged 6 commits into from
Jan 14, 2025
Merged

Remove %n in front and back of string #24

merged 6 commits into from
Jan 14, 2025

Conversation

jmslbam
Copy link
Contributor

@jmslbam jmslbam commented Sep 15, 2024

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 <br> which results in an address that's starts a line lower then needed.

Fixes #23

@jmslbam
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 `<br>` which results in an address that's starts a line lower then needed.

Fixes adamlc#23
adamlc
adamlc requested changes Sep 16, 2024
View reviewed changes
Copy link
Owner

@adamlc adamlc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for your PR!

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

@jmslbam
Copy link
Contributor Author

jmslbam commented Sep 16, 2024

@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
Copy link
Owner

adamlc commented Sep 17, 2024

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.

@jmslbam
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 `<br><br>`
@jmslbam
Update test when keeping attributes empty
2410afe 
So you don't end up with '%n%n' which gets converted to `<br><br>`
@jmslbam jmslbam requested a review from adamlc January 10, 2025 13:28
@jmslbam
Copy link
Contributor Author

jmslbam commented Jan 10, 2025

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
adamlc requested changes Jan 10, 2025
View reviewed changes
tests/FormatTest.php Outdated
$this->assertEquals(
$this->container->formatAddress(),
"Schulstrasse 4\n32547 Oyenhausen"
);
Copy link
Owner

@adamlc adamlc Jan 10, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

hmmz

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.

indention

indention-2

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

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

image

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done 👍

@jmslbam jmslbam requested a review from adamlc January 10, 2025 23:11
@adamlc adamlc merged commit b7071fe into adamlc:master Jan 14, 2025
1 check passed
@adamlc
Copy link
Owner

adamlc commented Jan 14, 2025

Amazing thanks for all your work @jmslbam

@jmslbam
Copy link
Contributor Author

jmslbam commented Jan 14, 2025

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@adamlc adamlc adamlc approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Don't prepend <br> if Recipient and or Organisation are empty
2 participants
@jmslbam @adamlc