Skip to content

Migrate @fluent/langneg to TypeScript#462

Merged
stasm merged 1 commit intoprojectfluent:masterfrom
stasm:ts-langneg
Mar 31, 2020
Merged

Migrate @fluent/langneg to TypeScript#462
stasm merged 1 commit intoprojectfluent:masterfrom
stasm:ts-langneg

Conversation

@stasm
Copy link
Copy Markdown
Contributor

@stasm stasm commented Mar 30, 2020

See #376.

import filterMatches from "./matches";
import {filterMatches} from "./matches";

function GetOption(options, property, type, values, fallback) {
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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