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

Removed maxLength limit for subdivision and updated description #479

Draft
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

arthur-clara
Copy link
Collaborator

What type of PR is this?

/kind feature

What this PR does / why we need it:

Updates the subdivision schema to remove the limitation to match ISO 3166-2 to allow subdivisions with more than two digits.

Which issue(s) this PR fixes:

Fixes #442

@arthur-clara arthur-clara requested a review from jacobyavis March 26, 2024 12:17
Copy link
Contributor

@JSv4 JSv4 left a comment

Choose a reason for hiding this comment

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

LGTM!

"type": "string",
"minLength": 1,
"maxLength": 3,
"pattern": "^[A-Z0-9]{1,3}$",
"pattern": "^[A-Z0-9]{1,}$",
Copy link
Member

Choose a reason for hiding this comment

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

@arthur-clara @JSv4 should we eliminate the pattern entirely? Now that this isn't ISO, who is to say that the only characters are latin alphabet A-Z and digits 0-9

@JSv4
Copy link
Contributor

JSv4 commented Mar 26, 2024

For some context, the ISO codes don't map cleanly to specific jurisdictions that relevant for the purposes of startup financings - e.g. Abu Dhabi and UK where separate subdivisions are relevant. @arthur-clara, can you add that example you used in detail? Would be good to have documented somewhere for reference.

@JSv4
Copy link
Contributor

JSv4 commented Mar 26, 2024

We're thinking that we need to separately handle subdivision name and subdivision code or some kind of split to handle ISO codes where they're of sufficient fidelity but capture finer grains where we need to. We could also use JSON Schema conditional validation to have special rules for certain countries.

Copy link
Contributor

@JSv4 JSv4 left a comment

Choose a reason for hiding this comment

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

See comments

@arthur-clara arthur-clara marked this pull request as draft June 20, 2024 13:14
@JSv4
Copy link
Contributor

JSv4 commented Jun 20, 2024

Per discussion on 6/20/24, we can use two properties here - one for codes (for US states) and one for name (where ISO codes don't make sense). Issuer and address schema would be impacted as they both reference the modified type.

@arthur-clara
Copy link
Collaborator Author

@JSv4 Per the discussion, I have reverted the subdivisionCode schema to remove and changes and added in a new object subdivisionName that is free text. I have also added "jurisdiction_of_formation" to the ISSUER object which can be used when the subdivision code does not work.

I am having an issue with the PR though because the generate docs step is failing for me and I am not sure why.

@JSv4
Copy link
Contributor

JSv4 commented Aug 15, 2024

@arthur-clara to add a oneOf validation

@arthur-clara
Copy link
Collaborator Author

@JSv4

I was finally looking at implementing the oneOf validation but I realized this would be a breaking change.

Right now, the country address type includes an optional country_subdivision property which points to the CountrySubdivisionCode schema.

I am adding country_subdivision_name property to address which points to a new CountrySubdivisionName schema

However, if I add the oneOf validation, it means that we now require country_subdivision or country_subdivision_name which I believe is a breaking change.

Are we okay with that or am I being obtuse and missing something?

@JSv4
Copy link
Contributor

JSv4 commented Oct 10, 2024

Makes sense. I say we call this. Becoming the longest little quick PR in all the known universe :-).

@arthur-clara
Copy link
Collaborator Author

arthur-clara commented Oct 10, 2024

Can I just add the country_subdivision_name as an optional to the address schema so that we can accommodate non-US?

Actually...it isn't in the address. It is in the Issuer that is the issue. country_subdivision_of_formation.

@lavens
Copy link
Contributor

lavens commented Oct 10, 2024

@arthur-clara this might be an approach to enforce a oneof for multiple optional values
https://stackoverflow.com/questions/60403128/json-schema-oneof-without-required

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.

[Enhancement]: Remove 2-digit length validation on CountrySubdivionCode
4 participants