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.

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

Examples of correct addresses in NL #126

Merged
merged 17 commits into from
Sep 7, 2021
Merged

Conversation

emacgillavry
Copy link
Contributor

@emacgillavry emacgillavry commented Jan 26, 2021

👋 I did some awesome work for the Pelias project and would love for everyone to have a look at it and provide feedback.


Here's the reason for this change 🚀

We'd like to contribute changes and additions to the resources to be able to validate correct addresses.


Here's what actually got changed 👏


Here's how others can test the changes 👀

All examples fail, hence we removed the 'test' pre-commit hook to be able to create a PR as a placeholder.

Fixes #127
Fixes #128
Fixes #129
Fixes #130
Fixes #131
Fixes #133
Fixes #137

package.json Outdated Show resolved Hide resolved
@Joxit
Copy link
Member

Joxit commented Sep 6, 2021

Hi @emacgillavry thank you for your PR
Can you rebase your code to master ? We need to trigger the CI again with GitHub Actions 😄

emacgillavry added a commit to webmapper/parser that referenced this pull request Sep 7, 2021
@emacgillavry
Copy link
Contributor Author

Did a rebase, but not too familiar with this workflow. Hope this works out ok. Otherwise, don't hesitate to reach me on gitter/pelias.

@missinglink
Copy link
Member

missinglink commented Sep 7, 2021

hi @emacgillavry, this is looking great, I see you've checked all the items off your list 👍

@Joxit there is an issue with GH Actions where it doesn't run Actions for contributors outside the org.
I can see why they set it up like that, to prevent abuse, but it's actually pretty annoying.

This is what it's currently set at for this repo:

Screenshot 2021-09-07 at 15 10 38

I tried this in the past with another repo and even after changing that and asking the contributor to push an empty commit, it still didn't trigger the Actions, so I ended up having to move the commits to another branch manually.

I'd like to fix this so that all public contributions trigger Actions, especially since the repos are open-source so we don't pay anything, let me know if you figure it out. 🤷

see: https://docs.github.com/en/github/administering-a-repository/managing-repository-settings/disabling-or-limiting-github-actions-for-a-repository#configuring-required-approval-for-workflows-from-public-forks

missinglink
missinglink previously approved these changes Sep 7, 2021
Copy link
Member

@missinglink missinglink left a comment

Choose a reason for hiding this comment

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

Adding my 'Approval' in attempt to trigger GH Actions.

note: please don't consider this an endorsement/approval of the work at this time.

@missinglink
Copy link
Member

I don't see this option:

actions

The org settings are the same BTW:

Screenshot 2021-09-07 at 15 23 15

@missinglink missinglink dismissed their stale review September 7, 2021 13:25

dismissing review, did not trigger Actions

@Joxit
Copy link
Member

Joxit commented Sep 7, 2021

Oh, thank you @missinglink for the intel, that's sad 😞

I saw one error with the rebase, so I will try to fix this with @emacgillavry and do my review

@missinglink
Copy link
Member

Oh look, it's working 🪄

@Joxit Joxit merged commit a1af69b into pelias:master Sep 7, 2021
@Joxit
Copy link
Member

Joxit commented Sep 7, 2021

Thank you again @emacgillavry for your great contribution and your time 😄

Copy link
Member

@missinglink missinglink left a comment

Choose a reason for hiding this comment

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

Late to the party on this one, couple minor comments.

Really great stuff, thanks for this!

@@ -15,6 +15,12 @@ class CompoundStreetClassifier extends WordClassifier {
// this removes suffixes such as 'r.' which can be ambiguous
minlength: 3
})

libpostal.load(this.suffixes, ['de', 'nl'], 'concatenated_suffixes_inseparable.txt', {
Copy link
Member

Choose a reason for hiding this comment

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

question: is de required here to get nl working or is this just nice to have since it solves the same problem in another language?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's the latter: it solve the same problem ;-)

{"zipex":"1234 AB,2490 AA","key":"NL","name":"NETHERLANDS","fmt":"%O%n%N%n%A%n%Z %C","require":"ACZ","zip":"[1-9][0-9]{3} ?(?!SA|SD|SS)[A-Z]{2}","posturl":"http://www.postnl.nl/voorthuis/","id":"data/NL"}
Copy link
Member

Choose a reason for hiding this comment

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

I recall us discussing this in the past, how this regex is better than the one sourced from the Chromium project.
One potential issue here is if we ever attempt to update the rest of them via the download script that this might get lost.

Since we haven't updated them so far I doubt its much of an issue, can we please just make sure this is covered with tests so they break if that happens and we investigate and fix it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have submitted #145 to track adding tests for this.

@missinglink
Copy link
Member

missinglink commented Sep 7, 2021

FYI: the npm module wasn't published due to that step being cancelled (not by me):

https://github.com/pelias/parser/runs/3535440988

Screenshot 2021-09-07 at 17 36 42

@Joxit
Copy link
Member

Joxit commented Sep 8, 2021

It seems that GitHub Actions had a Internal Error 😕

2021-09-08_09-38

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