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

Fixed Dutch postal code validation. #74

Merged
merged 6 commits into from
Oct 16, 2019
Merged

Fixed Dutch postal code validation. #74

merged 6 commits into from
Oct 16, 2019

Conversation

Cerbrus
Copy link
Contributor

@Cerbrus Cerbrus commented Oct 8, 2019

This resolves issue #73

  • Disabled left-padding on Dutch postal codes.
  • Dutch postal codes can not start with a 0.
  • Dutch postal codes can not contain SA, SS or SD.

I see this is a generated file, but it's origins are unclear to me. If this should be implemented elsewhere, I'm happy to add another pull request. :-)

This resolves issue #73

- Disabled left-padding on Dutch postal codes.
- Dutch postal codes can not start with a `0`.
- Dutch postal codes can not contain `SA`, `SS` or `SD`.

I see this is a generated file, but it's origins are unclear to me. If this should be implemented elsewhere, I'm happy to add another pull request. :-)
@Cerbrus
Copy link
Contributor Author

Cerbrus commented Oct 8, 2019

Apparently there are some unit tests in place that consider postal codes starting with a 0 to be valid. I'm not too sure how to update those tests without breaking other cases, since I can't get the project to run locally ("NuGet exited with code 3")

@mmaruniak
Copy link
Contributor

Sorry I missed your comments here, the correct place for the changes would be https://github.com/Cimpress-MCP/PostalCodes.Net/blob/master/src/countries/NL.json

About the unit tests, feel free to update the incorrect postal codes in the test cases. I guess it's only the ones starting with 0 right?

@Cerbrus
Copy link
Contributor Author

Cerbrus commented Oct 14, 2019

Thank you, @mmaruniak. I'll have a look at that.

Regenerated test files.
@Cerbrus
Copy link
Contributor Author

Cerbrus commented Oct 14, 2019

Turns out there was a rather old version of nuget added in the repository.
That version was unable to load a couple of packages. Updating nuget fixed that issue.
image

Then, I was able to build the projects and re-generate the relevant files.

Michiel added 3 commits October 14, 2019 09:44
(I'm not so sure about lines 11 & 12)
…sor implementation

Apparently the "GeneratePredecessor" implementation can't reliably handle generating a predecessor that wraps back to something other than 0.

9999 -> 0000 works fine, but when it has to wrap from 9999 -> 1000 and back, it doesn't know how to handle that.

I've disabled those test cases for now, as I'm not sure if I can fix them or how that would be done.
@Cerbrus
Copy link
Contributor Author

Cerbrus commented Oct 14, 2019

@mmaruniak: Build succeeded now, but I had to disable some tests that deal with predecessors. The method that generates a predecessor doesn't seem to be able to handle numeric values that don't wrap around to 0. I'm not sure how to fix that.

Remvoed some commented-out test cases.
@Cerbrus
Copy link
Contributor Author

Cerbrus commented Oct 15, 2019

Done, @mmaruniak. Thanks for the review!

@mmaruniak mmaruniak merged commit a46a9d7 into Cimpress-MCP:master Oct 16, 2019
mmaruniak added a commit that referenced this pull request Oct 16, 2019
Merge feature #74 into release 2.1.0
@Cerbrus
Copy link
Contributor Author

Cerbrus commented Oct 21, 2019

@mmaruniak: Thanks for the merge.

Do you happen to have some information on if / when a new version of the package can be released?

@mmaruniak
Copy link
Contributor

Sorry @Cerbrus we have a bit of a situation here. The API keys to publish the artifact are expired so I didn't manage to release it. And we are in process of transferring the repository to new owners so I'll get in touch with the original owners and try to fix the API keys

@Cerbrus
Copy link
Contributor Author

Cerbrus commented Oct 21, 2019

Oh wow, yea, I can imagine that taking some time. Thanks for the update!

@Cerbrus
Copy link
Contributor Author

Cerbrus commented Nov 21, 2019

@mmaruniak Is this still on the roadmap somewhere?
I just came across the ticket for this, on my end :-)

@mmaruniak
Copy link
Contributor

Right, sorry for not getting back to this sooner @Cerbrus . I am out for a week but I have transferred this repository to @pavelsavara . Pavel could you please try releasing this? I think we only need to trigger a build on the 2.1.0 release branch now

@pavelsavara
Copy link
Contributor

We just released version 3 for .NET standard 2.0. Is that ok for you ?
(we do not have .NET framework build pipeline right now) @Cerbrus

@Cerbrus
Copy link
Contributor Author

Cerbrus commented Nov 25, 2019

Hi @pavelsavara

I had to update the target framework of our project to 4.6.1, but that had to happen some day, any way.
Other than that minor tweak on our side, the package works like a charm!

Thanks for the update (@mmaruniak as well)!

pavelsavara pushed a commit that referenced this pull request Dec 18, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants