Merge .pyi files #926

chanicpanic · 2021-06-28T18:55:31Z

I started merging the stub files into the code. Here are a couple things I noticed so far:

In lexer.py the signatures of some methods of TraditionalLexer and ContextualLexer do not match the signatures in lexer.pyi.
lark.pyi and tree.pyi use typing.Literal and typing.Protocol which are new in Python 3.8. The mypy docs suggest using the typing_extensions package, but it may not be installed. How do we want to approach this?

One other question: Would you like me to add type annotations to more functions where possible, or do you only want them on public interfaces?

ast_utils, grammar, and indenter

load_grammar, reconstruct, and visitors

ThatXliner · 2021-06-28T19:04:57Z

lark.pyi and tree.pyi use typing.Literal and typing.Protocol which are new in Python 3.8. The mypy docs suggest using the typing_extensions package, but it may not be installed. How do we want to approach this?

My idea:

I feel like we can do something involving strings and typing.TYPE_CHECKING. Or we can keep those in .pyi files if approach 1 doesn't work.

lark/grammar.py

erezsh · 2021-06-28T19:22:29Z

I'd rather not add dependencies, even if really basic ones. It might affect the standalone parser.

I think we can keep those exceptions in .pyi files for now, until we find a better solution.

lark/grammar.py

erezsh · 2021-06-28T19:24:30Z

As for adding type annotations to internal code, it might make sense in situations where the type isn't obvious. But I prefer if we went through that on a separate PR.

chanicpanic · 2021-06-28T19:33:10Z

As for adding type annotations to internal code, it might make sense in situations where the type isn't obvious. But I prefer if we went through that on a separate PR.

Sounds good. I'll only apply the existing types from the .pyi's in this PR.

exceptions, lark, and tree

chanicpanic · 2021-06-28T23:27:22Z

An idea I had for Literal combining typing_extensions and typing.TYPE_CHECKING:

import sys
from typing import TYPE_CHECKING
if TYPE_CHECKING:
    if sys.version_info >= (3, 8):
        from typing import Literal
    else:
        from typing_extensions import Literal

x: 'Literal["open", "close"]' = "open"

However, the same probably won't work for Protocol because it is a class that is extended.

erezsh · 2021-06-29T06:06:03Z

What if we only support type checking for 3.8 and up? The code will still run with 3.6

chanicpanic · 2021-06-29T18:12:04Z

Type checking need not be limited to 3.8+. mypy lists typing_extensions as a dependency, so the new types should be available on whichever version of python the type checking is being run. The challenge is preventing the interpreter from complaining about undefined names for those who do not use type checking. This case is the reason why the type of x in the example above is provided as a string literal. It prevents python from trying to evaluate it.

Upon further inspection, I don't think PostLex should be a Protocol. Since #859, PostLex is an abstract class and thus provides nominal subtyping. So, I think we can use the approach given above for Literal and fully merge the stub files.

__init__, lexer, and parsers/__init__

MegaIng

A few comments. Not all of them are really directly solvable problems, but some are more a TODOs for a potentially future PR. I wouldn't be opposed if you decided to 'fix' none of these.

MegaIng · 2021-06-29T22:23:51Z

lark/common.py


+if TYPE_CHECKING:
+ from .lark import PostLex


I think we can just move the definition of PostLex into common.py. It fits better here anyway.

Why not lexer.py?

Btw this is an opportunity to change the postlex interface completely, if we wish to.

MegaIng · 2021-06-29T22:28:19Z

lark/lark.py

@@ -23,41 +19,51 @@
 except ImportError:
 regex = None

+
+###{standalone


@erezsh This can happen in another PR, but we might want to consider removing type annotations like we are removing docstrings from the standalone parser.

I agree that we should, if we can.

MegaIng · 2021-06-29T22:35:17Z

lark/lark.py

+ keep_all_tokens: bool
+ tree_class: Any
+ parser: 'Literal["earley", "lalr", "cyk", "auto"]'
+ lexer: 'Union[Literal["auto", "standard", "contextual", "dynamic", "dynamic_complete"], Type[Lexer]]'


We can instead define types inside a version guarded if-statement that when Literal is not available is just Union[str, Type[Lexer]] (and similar for all other types here). Not sure if it is better thou.

I prefer the current way. It provides the precision of Literal even when type checking on 3.6 and 3.7.

What does the current implementation do when typing_extensions is not installed?

The current implementation executes normally because the import of typing_extensions is wrapped in an if TYPE_CHECKING. I believe it is reasonable to assume that typing_extensions will be installed when TYPE_CHECKING is true because typing_extensions is a dependency of mypy.

MegaIng · 2021-06-29T22:37:45Z

lark/lexer.py

@@ -9,12 +10,23 @@
 ###{standalone
 from copy import copy

+from types import ModuleType
+from typing import (


We don't need to have duplicates imports inside of ###{standalone

MegaIng · 2021-06-29T22:40:12Z

lark/lexer.py

- raw = None
- type = None
+if TYPE_CHECKING:
+ from .common import LexerConf


Probably a TODO for later, but I think we can instead of guarding the circle with TYPE_CHECKING move more stuff into common.py. That is why it exists. ( In this case, the definition of TerminalDef/the Patterns/Token)

MegaIng · 2021-06-29T22:42:29Z

lark/lexer.py

 """Lexer interface

 Method Signatures:
 lex(self, text) -> Iterator[Token]
 """
- lex = NotImplemented
+ lex: Callable[..., Iterator[Token]] = NotImplemented


If we are inheriting from ABC, this should just be a normal abstractmethod.

MegaIng · 2021-06-29T22:44:09Z

lark/lexer.py

@@ -452,7 +494,7 @@ def __init__(self, conf, states, always_accept=()):
 def make_lexer_state(self, text):
 return self.root_lexer.make_lexer_state(text)

- def lex(self, lexer_state, parser_state):
+ def lex(self, lexer_state: Any, parser_state: Any) -> Iterator[Token]:


Another TODO for potentially later: corrects these annotations

MegaIng · 2021-06-29T22:45:25Z

lark/load_grammar.py

@@ -1072,7 +1082,7 @@ def _unpack_definition(self, tree, mangle):
 return name, exp, params, opts


- def load_grammar(self, grammar_text: str, grammar_name: str="<?>", mangle: Callable[[str], str]=None) -> None:
+ def load_grammar(self, grammar_text: str, grammar_name: str="<?>", mangle: Optional[Callable[[str], str]]=None) -> None:


Maybe I am wrong here, but isn't Optional automatically added when =None is the default?

Interesting, I didn't know about that feature. However, it looks like its use is no longer recommended. https://stackoverflow.com/questions/62732402/can-i-omit-optional-if-i-set-default-to-none

Aha. I didn't see that it was deprecated. Good to know.

MegaIng · 2021-06-30T19:58:09Z

lark/common.py

- postlex: 'PostLex' = None
- callbacks: Optional[Dict[str, _Callback]] = None
+ postlex: 'Optional[PostLex]' = None
+ callbacks: Dict[str, _Callback] = {}


Do you need to provide values when annotation on classes?

MegaIng · 2021-06-30T19:59:53Z

lark/lexer.py

- raw: str = None
- type: str = None
+ raw: Optional[str] = None
+ type: Optional[str] = None


This is not optional. It is also more of a ClassVar

erezsh · 2021-08-17T07:09:55Z

@chanicpanic Thanks a lot! I'm sure it was hard work. Sorry it took me so long to review.

One question, is __init__(...) -> None really necessary? It seems meaningless, and it can't really be anything else.

chanicpanic · 2021-08-17T17:09:55Z

@erezsh No problem. I'm glad I can help.

Annotating the return type of __init__ is optional as long as at least one of its arguments is annotated. The relevant documentation is at https://mypy.readthedocs.io/en/stable/class_basics.html#annotating-init-methods

erezsh · 2021-08-17T17:15:55Z

I see, thanks for explaining. It's a little surprising, if I already run mypy, why wouldn't I want to scan all the functions by default?

chanicpanic · 2021-08-17T17:30:55Z

I believe mypy behaves this way to make it easier for existing projects to adopt type annotations little by little. Annotations can be applied to the most important or straightforward functions first without receiving a bunch of nasty errors for those that have not been annotated. Also, some functions/variables may be so dynamic that it is hardly worth the trouble to try to declare precise types for them. Thus, you can opt to omit annotations and mypy won't complain.

The package was removed in PR lark-parser#926. When trying to install lark upstream, pip fails: > error: package directory 'lark-stubs' does not exist

chanicpanic added 2 commits June 28, 2021 10:00

Start merging .pyi files

8d0cdee

ast_utils, grammar, and indenter

Continue merging .pyi files

d7b819e

load_grammar, reconstruct, and visitors

erezsh reviewed Jun 28, 2021

View reviewed changes

lark/grammar.py Outdated Show resolved Hide resolved

MegaIng reviewed Jun 28, 2021

View reviewed changes

lark/grammar.py Outdated Show resolved Hide resolved

lark/grammar.py Outdated Show resolved Hide resolved

More .pyi merging

089bc2b

exceptions, lark, and tree

chanicpanic added 4 commits June 29, 2021 13:08

Last batch of .pyi files

433adee

__init__, lexer, and parsers/__init__

Merge the Literal types

be76059

Declare instance variable types at class level

bca7c79

Update mypy workflow

1c4af01

MegaIng reviewed Jun 29, 2021

View reviewed changes

Corrections for PR and some mypy errors

4bc9445

chanicpanic marked this pull request as ready for review June 30, 2021 20:01

MegaIng approved these changes Jun 30, 2021

View reviewed changes

Remove default values on type declarations and mark ClassVars

804114d

erezsh merged commit 3e2a61a into lark-parser:v1.0 Aug 17, 2021

michael-k added a commit to michael-k/lark that referenced this pull request Sep 23, 2021

Remove package lark-stubs from setup.py

f16be71

The package was removed in PR lark-parser#926. When trying to install lark upstream, pip fails: > error: package directory 'lark-stubs' does not exist

michael-k mentioned this pull request Sep 23, 2021

Remove package lark-stubs from setup.py #1007

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Merge .pyi files #926

Merge .pyi files #926

chanicpanic commented Jun 28, 2021

ThatXliner commented Jun 28, 2021

erezsh commented Jun 28, 2021

erezsh commented Jun 28, 2021 •

edited

Loading

chanicpanic commented Jun 28, 2021

chanicpanic commented Jun 28, 2021

erezsh commented Jun 29, 2021

chanicpanic commented Jun 29, 2021

MegaIng left a comment

MegaIng Jun 29, 2021

erezsh Jun 30, 2021

MegaIng Jun 29, 2021

erezsh Jun 30, 2021

MegaIng Jun 29, 2021

chanicpanic Jun 29, 2021

MegaIng Jun 30, 2021

chanicpanic Jun 30, 2021

MegaIng Jun 29, 2021

MegaIng Jun 29, 2021

MegaIng Jun 29, 2021

MegaIng Jun 29, 2021

MegaIng Jun 29, 2021

chanicpanic Jun 29, 2021

MegaIng Jun 30, 2021

MegaIng Jun 30, 2021

MegaIng Jun 30, 2021

erezsh commented Aug 17, 2021

chanicpanic commented Aug 17, 2021

erezsh commented Aug 17, 2021

chanicpanic commented Aug 17, 2021

Merge .pyi files #926

Merge .pyi files #926

Conversation

chanicpanic commented Jun 28, 2021

ThatXliner commented Jun 28, 2021

erezsh commented Jun 28, 2021

erezsh commented Jun 28, 2021 • edited Loading

chanicpanic commented Jun 28, 2021

chanicpanic commented Jun 28, 2021

erezsh commented Jun 29, 2021

chanicpanic commented Jun 29, 2021

MegaIng left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

erezsh commented Aug 17, 2021

chanicpanic commented Aug 17, 2021

erezsh commented Aug 17, 2021

chanicpanic commented Aug 17, 2021

erezsh commented Jun 28, 2021 •

edited

Loading