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

Added new features: vehicle license number, mail - county, phone number #49

Open
wants to merge 8 commits into
base: master
Choose a base branch
from

Conversation

storbukas
Copy link
Contributor

New features:

  • Vehicle license number
    • Validation
    • Geographical affiliation
    • Category (geographical affiliated, electric, embassy, etc.)
  • Phone number
    • Validation
    • Provider
    • Type (landline, mobile, m2m, etc.)
    • Status
  • Mail
    • County lookup

Copy link
Contributor

@janhoy janhoy left a comment

Choose a reason for hiding this comment

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

Er det noen grunn til at denne ikke er merget. Ser jo veldig bra ut, uten at jeg har gjort en nøye review eller test

@storbukas
Copy link
Contributor Author

Burde ikke være det, venter bare på godkjenning

@janhoy
Copy link
Contributor

janhoy commented Jan 5, 2023

Ping @landro @olemartin @eivindw @bekkadmin - er det noen som vedlikeholder dette prosjektet slik at issues og PR's følges opp? Er vel en fare for at folk vil slutte å bruke og bidra til prosjektet dersom det virker litt forlatt...

Jeg har muligens tenkt å bidra med noen nye ting men blir litt usikker når jeg ser en 2 år gammel PR ligge urørt...

@janhoy
Copy link
Contributor

janhoy commented Jan 7, 2023

Jeg har planer om å legge til støtte for historiske data for fylker/kommuner, samt funksjonalitet for å kunne slå opp gammelt kommunenavn og finne det nye (også på tvers av flere kommune-sammenslåinger). Men vil gjerne at denne merges først og at det avklares om man skal fortsette med en blanding av norsk og engelsk API (#63).

@eivinhb
Copy link
Member

eivinhb commented Jan 16, 2023

Grunnen til at denne ikke er merget er at jeg er usikker på om vi skal forvalte slike filer i et bibliotek på denne måten. Jeg er usikker på om det er korrekt at NoCommons skal være "kilden" til regnr-serier forvaltet av vegvesenet. For El-bil, feks, er det veldig mye nye bokstavkombinasjoner hele tiden. EC ble vel brukt opp på bare 9mnd.

@janhoy
Copy link
Contributor

janhoy commented Jan 16, 2023

Kanskje det er riktigere om biblioteket tar som input en Path eller InputStream til originale datafiler, som det da blir opp til brukeren å laste ned og holde oppdatert?

@eivinhb
Copy link
Member

eivinhb commented Jan 17, 2023

Ja, det blir jo det da. Men da må vi definere en eller annen form den filen må være på.
Jeg tenker at denne PR-en uansett burde splittes opp i domener. Alt på en gang er ikke veien å gå.

@janhoy
Copy link
Contributor

janhoy commented Jan 19, 2023

Enig. Splitt opp i tre nye PR. Bruk engelske klasse/metodenavn. Legg til en load/init funksjon som peker til ekstern fil.

Ekstern fil bør være identisk noe man kan laste ned fra nettet, og det må jo da dokumenteres hvordan man laster ned disse.

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