Skip to content
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

More updates to v1.0 #964

Merged
merged 15 commits into from
Aug 21, 2021
Merged

More updates to v1.0 #964

merged 15 commits into from
Aug 21, 2021

Conversation

erezsh
Copy link
Member

@erezsh erezsh commented Aug 17, 2021

Hopefully it's set up right this time.

@codecov-commenter
Copy link

codecov-commenter commented Aug 17, 2021

Codecov Report

Merging #964 (343c22e) into v1.0 (be3be18) will increase coverage by 0.00%.
The diff coverage is 68.88%.

Impacted file tree graph

@@           Coverage Diff           @@
##             v1.0     #964   +/-   ##
=======================================
  Coverage   87.63%   87.63%           
=======================================
  Files          49       49           
  Lines        6888     6893    +5     
=======================================
+ Hits         6036     6041    +5     
  Misses        852      852           
Flag Coverage Δ
unittests 87.63% <68.88%> (+<0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
lark/indenter.py 87.67% <14.28%> (ø)
lark/lark.py 83.77% <28.57%> (ø)
lark/lexer.py 93.33% <71.42%> (ø)
lark/common.py 95.34% <75.00%> (ø)
lark/exceptions.py 86.00% <100.00%> (ø)
lark/grammar.py 95.52% <100.00%> (ø)
lark/parse_tree_builder.py 97.22% <100.00%> (+0.01%) ⬆️
lark/parser_frontends.py 95.26% <100.00%> (+0.02%) ⬆️
lark/tools/standalone.py 89.62% <100.00%> (+0.30%) ⬆️
lark/tree.py 65.89% <100.00%> (ø)
... and 3 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update be3be18...343c22e. Read the comment docs.

@erezsh erezsh requested a review from MegaIng August 17, 2021 11:26
@erezsh
Copy link
Member Author

erezsh commented Aug 17, 2021

The Python3 grammar has been updated for compatibility with Lark.js

lark/exceptions.py Outdated Show resolved Hide resolved
lark/indenter.py Outdated Show resolved Hide resolved
def deserialize(self, data, memo, lexer_conf, callbacks, options):
parser_conf = ParserConf.deserialize(data['parser_conf'], memo)
parser = LALR_Parser.deserialize(data['parser'], memo, callbacks, options.debug)
parser_conf.callbacks = callbacks
return ParsingFrontend(lexer_conf, parser_conf, options, parser=parser)


# ... Continued later in the module
Copy link
Member

Choose a reason for hiding this comment

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

Why?

Copy link
Member Author

Choose a reason for hiding this comment

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

So it won't be part of the standalone code.

Copy link
Member

Choose a reason for hiding this comment

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

I would suggest not splitting up the class. Instead move the complete class further down.

Copy link
Member Author

Choose a reason for hiding this comment

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

It's also a bit strange to split the class in the middle. It breaks the indentation, and obfuscates the end of the standalone directive.

The only other place in Lark that happens is tree.Tree, and I'm not sure that it should stay that way.

But my mind isn't made up. If you think splitting the class using inheritance is too weird, I'll accept it.

@@ -18,6 +16,9 @@
else:
from typing_extensions import Literal

###{standalone
from collections import OrderedDict
Copy link
Member

Choose a reason for hiding this comment

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

Wasn't the goal to move all imports into standalone.py?

Copy link
Member Author

Choose a reason for hiding this comment

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

Not exactly, I was mostly concerned about repetitive imports.

OrderedDict is only used once (and it a little meaningless in Python3, I just didn't think removing it fits into this PR)

Copy link
Member

Choose a reason for hiding this comment

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

Fair enough. (Also OrderedDict is still needed for 3.6 on non-CPython.)

@erezsh erezsh merged commit c3fc3fa into v1.0 Aug 21, 2021
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.

3 participants