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

add numeric code types, make types more precise, add some types tests #259

Closed
wants to merge 11 commits into from

Conversation

osdiab
Copy link

@osdiab osdiab commented Sep 29, 2021

Supercedes #198 , closes #184

The tests may look wacky but included a couple comments at the top to explain how it works.

Also npm test fails right now with this:

Oops! Something went wrong! :(

ESLint: 7.32.0

Error: Cannot find module '@eslint/eslintrc'
Require stack:
- /Users/omar/code/open-source/node-i18n-iso-countries/node_modules/eslint/lib/cli-engine/cli-engine.js
- /Users/omar/code/open-source/node-i18n-iso-countries/node_modules/eslint/lib/eslint/eslint.js
- /Users/omar/code/open-source/node-i18n-iso-countries/node_modules/eslint/lib/eslint/index.js
- /Users/omar/code/open-source/node-i18n-iso-countries/node_modules/eslint/lib/cli.js
- /Users/omar/code/open-source/node-i18n-iso-countries/node_modules/eslint/bin/eslint.js
    at Function.Module._resolveFilename (node:internal/modules/cjs/loader:941:15)
    at Function.Module._load (node:internal/modules/cjs/loader:774:27)
    at Module.require (node:internal/modules/cjs/loader:1013:19)
    at require (/Users/omar/code/open-source/node-i18n-iso-countries/node_modules/v8-compile-cache/v8-compile-cache.js:159:20)
    at Object.<anonymous> (/Users/omar/code/open-source/node-i18n-iso-countries/node_modules/eslint/lib/cli-engine/cli-engine.js:33:5)
    at Module._compile (/Users/omar/code/open-source/node-i18n-iso-countries/node_modules/v8-compile-cache/v8-compile-cache.js:192:30)
    at Object.Module._extensions..js (node:internal/modules/cjs/loader:1138:10)
    at Module.load (node:internal/modules/cjs/loader:989:32)
    at Function.Module._load (node:internal/modules/cjs/loader:829:14)
    at Module.require (node:internal/modules/cjs/loader:1013:19)
make: *** [eslint] Error 2


export type StringNumericCode =
Copy link
Author

Choose a reason for hiding this comment

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

pulled these numbers from wikipedia

{
"compilerOptions": {
"strict": true,
"noEmit": true
Copy link
Author

Choose a reason for hiding this comment

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

This keeps it from actually emitting any code

"strict": true,
"noEmit": true
},
"include": ["test/**/*.ts"],
Copy link
Author

Choose a reason for hiding this comment

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

This tells TypeScript to specifically try to compile the test files, and fails if the test files contain type errors

@@ -15,55 +13,53 @@ export type LocaleData = {
}
};

type Normalize<Input extends string | number> = Input extends string ? Lowercase<Input> : Input;
Copy link
Author

@osdiab osdiab Sep 29, 2021

Choose a reason for hiding this comment

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

Normalization is necessary since the library can actually accept any case of codes, like UsA or usa

export function alpha3ToNumeric(alpha3: string | Alpha3Code): string;
export function numericToAlpha2(numeric: number | string): string;
export function numericToAlpha3(numeric: number | string): string;
export function alpha2ToAlpha3<Input extends string>(alpha2: Input): Normalize<Input> extends Normalize<Alpha2Code> ? Alpha3Code : Alpha3Code | undefined;
Copy link
Author

Choose a reason for hiding this comment

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

But it always returns the upper-cased ones, so we dont normalize the return values


/**
* @param countryCode Alpha2 or Alpha3 or Numeric
* @param lang ISO 639-1 format string
*/
export function getName(
countryCode: string | number | Alpha2Code | Alpha3Code,
Copy link
Author

Choose a reason for hiding this comment

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

These kinds of types are redundant, since string and number already subsume Alpha2Code, Alpha3Code, and NumericCode

countryCode: string | number | Alpha2Code | Alpha3Code,
lang: string
): string;
export function getName(countryCode: string | number, lang: string): string | undefined;
Copy link
Author

Choose a reason for hiding this comment

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

We can't have a more specific type here since A) the name of the country varies arbitrarily by language; and B) if the language is bad, then it'll return undefined; I think it's a bad idea to try to add all the possible language codes, it's overkill.


/**
* @param lang ISO 639-1 format string
*/
export function getNames(lang: string): LocalizedCountryNames<{ select: 'official' }>;
export function getNames(lang: string): LocalizedCountryNames<{ select: 'official' }> | {};
Copy link
Author

Choose a reason for hiding this comment

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

Like in the other PR, I personally don't mind it having the {} empty object type. Ideally the API would have chosen something more explicit like null as the return type since it's easier to work with and way more explicit, but it's too late for that.

@@ -0,0 +1,128 @@
import { alpha2ToAlpha3, alpha2ToNumeric, alpha3ToAlpha2, alpha3ToNumeric, toAlpha2, toAlpha3 } from "..";
Copy link
Author

@osdiab osdiab Sep 29, 2021

Choose a reason for hiding this comment

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

The goal with these tests are to just make a program that doesn't contain type errors, but ensures that inference is happening properly when using the library functions.

@michaelwittig
Copy link
Owner

This PR seems to be quiet complicated. I'm preferring #319 for simplicity. Please reopen and explain me why this is better )I'm not using TypeScript).

@osdiab
Copy link
Author

osdiab commented Apr 13, 2023

i just stopped using this library since its types are bad, this PR was from a year and a half ago so I'm not fresh on the details, but based on rereading it, it basically made the returned and requested types much more precise and matching the intent of the documentation. The other one basically seems to pave over one inconsistency, but otherwise leaves it as is, which is an improvement for sure. But yeah, since i've given up on this library, and it isn't a priority for this library to have precise types, I don't plan to reopen this.

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.

Incorrect types for some functions?
2 participants