Skip to content

Add type hints to fluent.syntax & fluent.runtime - #180

Merged
eemeli merged 11 commits into
mainfrom
hints
Mar 13, 2023
Merged

Add type hints to fluent.syntax & fluent.runtime#180
eemeli merged 11 commits into
mainfrom
hints

Conversation

@eemeli

Copy link
Copy Markdown
Member

Fixes#165

The work here is split into commits that may be easier to review separately. The internal hints for the runtime resolver and compiler are pretty weak, as the base implementation is making explicit use of the language being fundamentally untyped.

The bugs that are fixed are all various sorts of corner cases that the code didn't account for previously; the errors tend to follow similar cases in fluent.js.

While adding the types I used both mypy and pylance; the added CI tests only apply mypy --strict as the strict-mode pylance checks don't all pass (mostly due to using Unknown values in places that accept Any).

@eemeli
eemeliforce-pushed the hints branch 5 times, most recently from b2f8bb4 to c498d15CompareFebruary 11, 2023 14:19
@parmenashp

Copy link
Copy Markdown

Are you planning on bumping to 0.19 after this?

@eemeli

Copy link
Copy Markdown
MemberAuthor

Yes.

@eviljeffeviljeff left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Just random nits/comments that should be taken as suggestions at most.

Comment threadfluent.syntax/fluent/syntax/visitor.py Outdated
Comment threadfluent.runtime/fluent/runtime/bundle.py Outdated
return match(val2, val1, env)

return val1 == val2
return cast(bool, val1 == val2)

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

out of interest, why do we have to cast an equality expression to bool? Isn't an equality expression implicitly a boolean?

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

It's because the inputs are Any, and that at least technically allows for an __eq__(self, other) overload that returns something other than a bool.

maximumFractionDigits = attr.ib(default=None)
minimumSignificantDigits = attr.ib(default=None)
maximumSignificantDigits = attr.ib(default=None)
style: Literal['decimal', 'currency', 'percent'] = attr.ib(

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

should we use the FORMAT_STYLE_DECIMAL etc constants here instead?

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

I don't think that's possible with Literal[], or at least I'm not aware of a way of an alternative type expression that would resolve to this.

Co-authored-by: Andrew Williamson <awilliamson@mozilla.com>
@eemeli
eemeli removed the request for review from gregtatumMarch 11, 2023 09:26
@eemeli
eemeli merged commit 02906ae into mainMar 13, 2023
@eemeli
eemeli deleted the hints branch March 13, 2023 14:03
Sign up for freeto 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.

Add type hinting

3 participants

@eemeli@parmenashp@eviljeff