- 
                Notifications
    
You must be signed in to change notification settings  - Fork 30
 
Improve typing #181
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
base: main
Are you sure you want to change the base?
Improve typing #181
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -1,10 +1,12 @@ | ||||||||||||||||||||||
| import attr | ||||||||||||||||||||||
| import contextlib | ||||||||||||||||||||||
| from typing import Any, Dict, Generator, List, Set, TYPE_CHECKING, Union, cast | ||||||||||||||||||||||
| from typing import TYPE_CHECKING, Any, Dict, Iterable, Iterator, List, Mapping, Sequence, Set, Union, cast | ||||||||||||||||||||||
| from typing_extensions import Final, Self | ||||||||||||||||||||||
| 
     | 
||||||||||||||||||||||
| import attr | ||||||||||||||||||||||
| from fluent.syntax import ast as FTL | ||||||||||||||||||||||
| 
     | 
||||||||||||||||||||||
| from .errors import FluentCyclicReferenceError, FluentFormatError, FluentReferenceError | ||||||||||||||||||||||
| from .types import FluentType, FluentNone, FluentInt, FluentFloat | ||||||||||||||||||||||
| from .types import FluentFloat, FluentInt, FluentNone, FluentType | ||||||||||||||||||||||
| from .utils import reference_to_id, unknown_reference_error_obj | ||||||||||||||||||||||
| 
     | 
||||||||||||||||||||||
| if TYPE_CHECKING: | ||||||||||||||||||||||
| 
        
          
        
         | 
    @@ -27,7 +29,7 @@ | |||||||||||||||||||||
| 
     | 
||||||||||||||||||||||
| 
     | 
||||||||||||||||||||||
| # Prevent expansion of too long placeables, for memory DOS protection | ||||||||||||||||||||||
| MAX_PART_LENGTH = 2500 | ||||||||||||||||||||||
| MAX_PART_LENGTH: Final = 2500 | ||||||||||||||||||||||
| 
     | 
||||||||||||||||||||||
| 
     | 
||||||||||||||||||||||
| @attr.s | ||||||||||||||||||||||
| 
          
            
          
           | 
    @@ -56,7 +58,7 @@ class ResolverEnvironment: | |||||||||||||||||||||
| current: CurrentEnvironment = attr.ib(factory=CurrentEnvironment) | ||||||||||||||||||||||
| 
     | 
||||||||||||||||||||||
| @contextlib.contextmanager | ||||||||||||||||||||||
| def modified(self, **replacements: Any) -> Generator['ResolverEnvironment', None, None]: | ||||||||||||||||||||||
| def modified(self, **replacements: Any) -> Iterator[Self]: | ||||||||||||||||||||||
| """ | ||||||||||||||||||||||
| Context manager that modifies the 'current' attribute of the | ||||||||||||||||||||||
| environment, restoring the old data at the end. | ||||||||||||||||||||||
| 
        
          
        
         | 
    @@ -68,7 +70,9 @@ def modified(self, **replacements: Any) -> Generator['ResolverEnvironment', None | |||||||||||||||||||||
| yield self | ||||||||||||||||||||||
| self.current = old_current | ||||||||||||||||||||||
| 
     | 
||||||||||||||||||||||
| def modified_for_term_reference(self, args: Union[Dict[str, Any], None] = None) -> Any: | ||||||||||||||||||||||
| def modified_for_term_reference(self, | ||||||||||||||||||||||
| args: Union[Mapping[str, Any], None] = None | ||||||||||||||||||||||
| ) -> 'contextlib._GeneratorContextManager[Self]': | ||||||||||||||||||||||
| return self.modified(args=args if args is not None else {}, | ||||||||||||||||||||||
| error_for_missing_arg=False) | ||||||||||||||||||||||
| 
     | 
||||||||||||||||||||||
| 
          
            
          
           | 
    @@ -99,7 +103,7 @@ class Message(FTL.Entry, BaseResolver): | |||||||||||||||||||||
| def __init__(self, | ||||||||||||||||||||||
| id: 'Identifier', | ||||||||||||||||||||||
| value: Union['Pattern', None] = None, | ||||||||||||||||||||||
| attributes: Union[List['Attribute'], None] = None, | ||||||||||||||||||||||
| attributes: Union[Iterable['Attribute'], None] = None, | ||||||||||||||||||||||
| comment: Any = None, | ||||||||||||||||||||||
| **kwargs: Any): | ||||||||||||||||||||||
| super().__init__(**kwargs) | ||||||||||||||||||||||
| 
        
          
        
         | 
    @@ -116,7 +120,7 @@ class Term(FTL.Entry, BaseResolver): | |||||||||||||||||||||
| def __init__(self, | ||||||||||||||||||||||
| id: 'Identifier', | ||||||||||||||||||||||
| value: 'Pattern', | ||||||||||||||||||||||
| attributes: Union[List['Attribute'], None] = None, | ||||||||||||||||||||||
| attributes: Union[Iterable['Attribute'], None] = None, | ||||||||||||||||||||||
| comment: Any = None, | ||||||||||||||||||||||
| **kwargs: Any): | ||||||||||||||||||||||
| super().__init__(**kwargs) | ||||||||||||||||||||||
| 
        
          
        
         | 
    @@ -129,7 +133,7 @@ class Pattern(FTL.Pattern, BaseResolver): | |||||||||||||||||||||
| # Prevent messages with too many sub parts, for CPI DOS protection | ||||||||||||||||||||||
| MAX_PARTS = 1000 | ||||||||||||||||||||||
| 
     | 
||||||||||||||||||||||
| elements: List[Union['TextElement', 'Placeable']] # type: ignore | ||||||||||||||||||||||
| elements: Sequence[Union['TextElement', 'Placeable']] | ||||||||||||||||||||||
| 
     | 
||||||||||||||||||||||
| def __init__(self, *args: Any, **kwargs: Any): | ||||||||||||||||||||||
| super().__init__(*args, **kwargs) | ||||||||||||||||||||||
| 
          
            
          
           | 
    @@ -198,14 +202,14 @@ def __call__(self, env: ResolverEnvironment) -> str: | |||||||||||||||||||||
| 
     | 
||||||||||||||||||||||
| 
     | 
||||||||||||||||||||||
| class NumberLiteral(FTL.NumberLiteral, BaseResolver): | ||||||||||||||||||||||
| value: Union[FluentFloat, FluentInt] # type: ignore | ||||||||||||||||||||||
| value: Union[FluentFloat, FluentInt] # type: ignore[assignment] | ||||||||||||||||||||||
| 
     | 
||||||||||||||||||||||
| def __init__(self, value: str, **kwargs: Any): | ||||||||||||||||||||||
| super().__init__(value, **kwargs) | ||||||||||||||||||||||
| if '.' in cast(str, self.value): | ||||||||||||||||||||||
| self.value = FluentFloat(self.value) | ||||||||||||||||||||||
| self.value = FluentFloat(cast(str, self.value)) | ||||||||||||||||||||||
| else: | ||||||||||||||||||||||
| self.value = FluentInt(self.value) | ||||||||||||||||||||||
| self.value = FluentInt(cast(str, self.value)) | ||||||||||||||||||||||
                
       | 
||||||||||||||||||||||
| if '.' in cast(str, self.value): | |
| self.value = FluentFloat(self.value) | |
| self.value = FluentFloat(cast(str, self.value)) | |
| else: | |
| self.value = FluentInt(self.value) | |
| self.value = FluentInt(cast(str, self.value)) | |
| if '.' in value: | |
| self.value = FluentFloat(value) | |
| else: | |
| self.value = FluentInt(value) | 
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.
Agreed. However, without the cast to str and with the change I made to FluentInt, it's trying to pass what it thinks is a Union[FluentFloat, FluentInt] to FluentInt() and marks it as an error. This should probably be cleaned up to something like this:
original_value = cast(str, self.value)
if '.' in original_value:
    self.value = FluentFloat(original_value)
else:
    self.value = FluentInt(original_value)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.
The diff is a perhaps a bit misleading. The argument value is already properly recognised as a str, and it's assigned to self.value in the super().__init__(). So we can just use that directly when re-setting the self.value.
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 had thought of that but thought that manipulation of value could happen in the future in FTL.NumberLiteral or BaseResolver, so I didn't want to assume too much. Do you foresee that happening? If not, using value directly is the better solution.
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.
That sounds like it'd be excessive magic. Better to use value directly to add an extra hurdle for such shenanigans.
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.
Just read the other day that a
stris also anIterable{str], we should avoid that type if we really want multiple strings.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.
In a loosely typed language like Python, I'm not sure this can be avoided. There are two options, in my mind: use a union of a bunch of common invariant collections (
Union[List[str], Tuple[str], AbstractSet[str]]) or add a runtime check to ensure that a string isn't being passed. I really don't like the first option because you're limiting what the user can pass in (aList[MyStringSubtype]can't be passed in, for instance), which is why I changed this toSequence[str]andIterable[str].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.
A third option is to convert from
strto alist()(which is done in a couple of other places):