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

Migrate @fluent/langneg to TypeScript #462

Merged
merged 1 commit into from
Mar 31, 2020
Merged

Conversation

stasm
Copy link
Contributor

@stasm stasm commented Mar 30, 2020

See #376.


function GetOption(options, property, type, values, fallback) {
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 removed this function because I don't think the implementation of @fluent/langneg needs to be 100% defensive. For TypeScript consumers, TS already gives us some of those guaranties. For non-TS consumers, we don't usually verify all inputs in other @fluent packages as thoroughly as this function did it.

resolvedReqLoc,
resolvedAvailLoc, strategy
Array.from(Object(requestedLocales)).map(String),
Array.from(Object(availableLocales)).map(String),
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'm tempted to remove the string and array normalization here as well, because again, I think we can make reasonable assumptions about the quality of the input. There are tests which fail if I do that, however.

for (const strategy in data) {
for (const groupName in data[strategy]) {
const group = data[strategy][groupName];
test(`${strategy} - ${groupName}`, () => {
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 moved the test call into the nested for loop so that each case is run as a separate Mocha test, resulting in better test summary and error logging.

@stasm stasm requested a review from zbraniecki March 30, 2020 12:51
@stasm stasm mentioned this pull request Mar 30, 2020
7 tasks
@stasm
Copy link
Contributor Author

stasm commented Mar 30, 2020

@zbraniecki Would you have a moment to review this? Other than adding types, I also made a few small changes to the implementation to make it a bit less defensive.

@stasm
Copy link
Contributor Author

stasm commented Mar 31, 2020

Thanks for the review, @zbraniecki!

@stasm stasm merged commit 49347e7 into projectfluent:master Mar 31, 2020
@stasm stasm deleted the ts-langneg branch March 31, 2020 11:09
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.

2 participants