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

Draft of Version 1.0 #925

Merged
merged 45 commits into from
Sep 13, 2021
Merged

Draft of Version 1.0 #925

merged 45 commits into from
Sep 13, 2021

Conversation

erezsh
Copy link
Member

@erezsh erezsh commented Jun 27, 2021

Changes so far

  • Removed deprecated code
  • Removed most of the code for Python 2 compatibility.

@codecov-commenter
Copy link

codecov-commenter commented Jun 27, 2021

Codecov Report

Merging #925 (77b07bb) into master (82ad512) will increase coverage by 0.51%.
The diff coverage is 92.72%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #925      +/-   ##
==========================================
+ Coverage   87.12%   87.63%   +0.51%     
==========================================
  Files          50       49       -1     
  Lines        6804     6893      +89     
==========================================
+ Hits         5928     6041     +113     
+ Misses        876      852      -24     
Flag Coverage Δ
unittests 87.63% <92.72%> (+0.51%) ⬆️

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

Impacted Files Coverage Δ
lark/ast_utils.py 0.00% <0.00%> (ø)
lark/parsers/lalr_interactive_parser.py 77.77% <ø> (-0.69%) ⬇️
setup.py 0.00% <ø> (ø)
lark/indenter.py 87.67% <81.81%> (-5.67%) ⬇️
lark/tree.py 65.89% <84.37%> (+3.17%) ⬆️
lark/exceptions.py 86.00% <88.00%> (+0.70%) ⬆️
lark/lark.py 83.72% <89.65%> (+1.93%) ⬆️
lark/common.py 95.34% <93.33%> (+4.17%) ⬆️
lark/lexer.py 93.35% <96.62%> (+1.30%) ⬆️
lark/__init__.py 100.00% <100.00%> (ø)
... and 13 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 82ad512...77b07bb. Read the comment docs.

@erezsh
Copy link
Member Author

erezsh commented Jun 27, 2021

Tasks remaining:

  • Merge .pyi files into the code, and remove them.
  • Update the interface to be more self-consistent (ideas?)
  • Improve the code with Python3 features, where possible (optional/low priority)
  • Anything else?

Copy link
Member

@MegaIng MegaIng left a comment

Choose a reason for hiding this comment

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

A few initial comments. I will take another look or too and maybe even just implement everything mentioned here in my own commits.

.github/workflows/tests.yml Outdated Show resolved Hide resolved
@@ -193,7 +191,7 @@ def __init__(self, token, expected, considered_rules=None, state=None, interacti
# TODO considered_rules and expected can be figured out using state
self.line = getattr(token, 'line', '?')
self.column = getattr(token, 'column', '?')
self.pos_in_stream = getattr(token, 'pos_in_stream', None)
self.pos_in_stream = getattr(token, 'start_pos', None)
Copy link
Member

Choose a reason for hiding this comment

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

We can just rename the attribute.

Copy link
Member Author

Choose a reason for hiding this comment

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

You mean, in the exceptions?

I'm less concerned about that (and anyway, start_pos wouldn't be a good name in this case). But I'm open to suggestions.

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. Keep pos_in_stream

lark/lark.py Outdated Show resolved Hide resolved
lark/tools/standalone.py Show resolved Hide resolved
lark/utils.py Outdated Show resolved Hide resolved
erezsh and others added 2 commits June 28, 2021 14:35
@ThatXliner
Copy link
Contributor

ThatXliner commented Jun 28, 2021

About the type-merging, you can possibly use https:/ilevkivskyi/com2ann to convert type comments to type annotations. I remember finding a repo that can merge type annotations... I'll get back when I remember it

@MegaIng
Copy link
Member

MegaIng commented Jun 28, 2021

@ThatXliner We are not using Type comments. We have separate stub files.

@ThatXliner
Copy link
Contributor

@ThatXliner We are not using Type comments. We have separate stub files.

Oh right. My bad.

I found https:/ambv/retype but it wasn't what I remembered. Hope it helps, though

Copy link
Member

@MegaIng MegaIng left a comment

Choose a reason for hiding this comment

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

(I think setup.py also needs to be updated for py36)

load_grammar, reconstruct, and visitors
@chanicpanic
Copy link
Contributor

Here is a list of compatibility/deprecated code I have come across in case you want to remove some of it.

@erezsh
Copy link
Member Author

erezsh commented Sep 12, 2021

Fixes some of the items @chanicpanic mentioned in this PR - #993

Regarding

  • __visit_tokens__ - To be honest I'm not sure why it bothered us. I think it can stay the way it is.

  • deprecation of lexer callbacks - I used to think that maybe the transformer parameter should be enough, since we added support for tokens. But it's still useful for going over ignored tokens, so unless we let transformer run on ignored tokens too, I think we should leave it.

  • UnexpectedToken.expected - Maybe it's still worth keeping? I have no strong opinion atm

@erezsh
Copy link
Member Author

erezsh commented Sep 12, 2021

Btw, @chanicpanic & @MegaIng,

I think we should just merge this, and continue work over the master branch. Any objections?

@MegaIng
Copy link
Member

MegaIng commented Sep 12, 2021

That would mean that we are not going to release any other version before 1.0, not even a 0.12.1 release. But that would be fine with me.

@erezsh
Copy link
Member Author

erezsh commented Sep 12, 2021

We can always continue working on a 0.12 branch. But I'm not sure that we need to.

@MegaIng
Copy link
Member

MegaIng commented Sep 12, 2021

Fair enough. Merge is good IMO. (Or do we also want to rename to main in the process?)

@erezsh
Copy link
Member Author

erezsh commented Sep 12, 2021

Or do we also want to rename to main in the process?

Seems pointless to me

@MegaIng
Copy link
Member

MegaIng commented Sep 12, 2021

Seems pointless to me

Fair enough. I dont have a strong opinion either, but this would be a prime opportunity.

@chanicpanic
Copy link
Contributor

No objections to merging from me.

@erezsh
Copy link
Member Author

erezsh commented Sep 12, 2021

Alright. I'll do it tomorrow then.

@erezsh erezsh merged commit efc881a into master Sep 13, 2021
@erezsh
Copy link
Member Author

erezsh commented Sep 13, 2021

Lark master is now officially Python 3 only!

@erezsh erezsh deleted the v1.0 branch September 16, 2021 14:45
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.

6 participants