-
Notifications
You must be signed in to change notification settings - Fork 573
Bug 1685565: Type @fluent/syntax #1902
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
Conversation
b756e34 to
005c66a
Compare
Pike
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is ready for review. I left an inline comment on core/utils/fluent/types.js with an open question.
The tests expect and if not pattern is because neither flow nor typescript narrow types on failed test assertions. Thus I went for test-and-return instead.
frontend/src/modules/fluenteditor/components/rich/RichTranslationForm.js
Outdated
Show resolved
Hide resolved
f3e9a60 to
1ee9083
Compare
|
Addressed the comments and discussions, TS errors are going from 137 to 105 now that we don't convert tests. There's one in-test fix about the mocked bundles, which is one of those cases where having type checks avoids having junk in tests. Just adding that as a data point, I don't think this puts a dent in our decision to leave the tests in JavaScript. |
| activeTranslationString || entity.original, | ||
| ); | ||
|
|
||
| if (syntax === '') { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why are these checks no longer needed? (there is the same check line#208)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
getSyntaxType never returns an empty string, it's always one of the SyntaxType enum.
This condition will always return 'false' since the types 'SyntaxType' and '""' have no overlap.
is what https://github.com/mozilla/pontoon/runs/2520885795?check_suite_focus=true#step:16:782 says on master right now.
|
Flagging myself for review here to take it through my standard manual editor testing process. |
mathjazz
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I haven't spot any issues.
The code also looks good to my newbie TS self.
@abowler2 Did you want to have another look here?
Very WIP.
Practically, I'm down to 2 flow errors, and passing tests.
Theoretically, I'm doing typing quite a bit different than it's done on the typescript side.
I remember conversations with @stasm about the use of base classes for Expressions etc and their incompleteness. Which I've done the other way around now so that refinements work.