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

Add NL parsing rules #72

Merged
merged 2 commits into from
Oct 28, 2024
Merged

Add NL parsing rules #72

merged 2 commits into from
Oct 28, 2024

Conversation

Pitchiu
Copy link
Collaborator

@Pitchiu Pitchiu commented Oct 24, 2024

This PR adds parsing rules for the Netherlands.

@Pitchiu Pitchiu requested a review from norgevz October 24, 2024 20:29
@Pitchiu
Copy link
Collaborator Author

Pitchiu commented Oct 24, 2024

@norgevz could you take a look?

@Pitchiu Pitchiu requested a review from skyman503 October 24, 2024 20:30
skyman503
skyman503 requested changes Oct 25, 2024
View reviewed changes
Copy link
Collaborator

@skyman503 skyman503 left a comment

Choose a reason for hiding this comment

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

Shouldn't there be some sort of separator between the building and unit? I found and example where the address was 32-6 and proposed change would use it as 326 which seems wrong.

@Pitchiu
Copy link
Collaborator Author

Pitchiu commented Oct 25, 2024

Shouldn't there be some sort of separator between the building and unit? I found and example where the address was 32-6 and proposed change would use it as 326 which seems wrong.

Correct, let's use hyphen since there is no hard rule for that in NL.

@Pitchiu Pitchiu requested a review from skyman503 October 25, 2024 10:56
@Pitchiu
Copy link
Collaborator Author

Pitchiu commented Oct 25, 2024

@skyman503, @norgevz suggestions applied. PR ready for the review.

norgevz
norgevz approved these changes Oct 28, 2024
View reviewed changes
Copy link
Collaborator

@norgevz norgevz left a comment

Choose a reason for hiding this comment

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

LGTM!

@Pitchiu Pitchiu merged commit cd7757d into main Oct 28, 2024
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@norgevz norgevz norgevz approved these changes

@skyman503 skyman503 Awaiting requested review from skyman503

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

Successfully merging this pull request may close these issues.
3 participants
@Pitchiu @norgevz @skyman503