FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

Fix comments in REPL - fixes #78 by ncw · Pull Request #79 · go-python/gpython · GitHub

/ gpython Public

Fix comments in REPL - fixes #78 - #79

Merged
ncw merged 1 commit into
masterfrom
fix-78-repl
Sep 28, 2019
ConversationCommits (1)ChecksFiles changed
Merged

Fix comments in REPL - fixes #78#79
ncw merged 1 commit into
masterfrom
fix-78-repl

Conversation

ncw commented Sep 15, 2019

Copy link
Copy Markdown
Collaborator

Before this change, entering a comment in the REPL caused the REPL to
read the comment indefinitely effectively breaking it.

After this change the behaviour should be exactly the same as python3/

ncw commented Sep 15, 2019

Copy link
Copy Markdown
Collaborator Author

Can you review this for me @corona10 ?

ncw mentioned this pull request Sep 15, 2019
Closed

codecov-io commented Sep 15, 2019 •
edited
Loading

Copy link
Copy Markdown

Codecov Report

Merging #79 into master will increase coverage by <.01%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master      #79      +/-   ##
==========================================
+ Coverage   68.65%   68.66%   +<.01%     
==========================================
  Files          59       59              
  Lines       10525    10528       +3     
==========================================
+ Hits         7226     7229       +3     
  Misses       2790     2790              
  Partials      509      509
Impacted Files Coverage Δ
repl/repl.go 100% <100%> (ø) ⬆️
symtable/symtable.go 94.23% <0%> (ø) ⬆️

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 eb115a9...9a4df56. Read the comment docs.

corona10 reviewed Sep 15, 2019
View reviewed changes

corona10 left a comment •
edited
Loading

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.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

This patch will fix this issue #78 .
But the input of '#' should be handled as same as below code?
Should we update grammar file for this?

[Gpython dev]
- os/arch: darwin/amd64
- go version: go1.10
>>> def a(): pass
>>>

corona10 commented Sep 15, 2019

Copy link
Copy Markdown
Collaborator

This is the example of pypy and rustpython.

Python 3.6.1 (dab365a465140aa79a5f3ba4db784c4af4d5c195, Feb 18 2019, 10:53:27)
[PyPy 7.0.0-alpha0 with GCC 4.2.1 Compatible Apple LLVM 10.0.0 (clang-1000.11.45.5)] on darwin
Type "help", "copyright", "credits" or "license" for more information.
And now for something completely different: ``"it's likely temporary until
forever" arigo''
>>>> #
>>>> #
>>>>
Welcome to the magnificent Rust Python 0.1.0 interpreter 😱 🖖
>>>>> #
>>>>>

corona10 commented Sep 15, 2019

Copy link
Copy Markdown
Collaborator

And it still doesn't work well on my local laptop with branch fix-78-repl bcb3785

Python 3.4.0 (none, unknown)
[Gpython dev]
- os/arch: darwin/amd64
- go version: go1.10
>>> #
... #
...

ncw commented Sep 15, 2019

Copy link
Copy Markdown
Collaborator Author

And it still doesn't work well on my local laptop with branch fix-78-repl bcb3785

Python 3.4.0 (none, unknown)
[Gpython dev]
- os/arch: darwin/amd64
- go version: go1.10
>>> #
... #
...

Ah you are right this is broken :-(

The pypy and rustpython ways of doing it are

  • a whole lot more logical in my opinion
  • much easier that emulating python3 exactly

Shall I make it work like that?

corona10 commented Sep 15, 2019

Copy link
Copy Markdown
Collaborator

@ncw
Yes, please.
IMHO, we don't have to follow everything of CPython :)

Fix comments in REPL - fixes #78
9a4df56 
Before this change, entering a comment in the REPL caused the REPL to
read the comment indefinitely effectively breaking it.

After this change the behaviour is the same as pypy.  The cpython
behaviour is slightly different printing a '...' after a comment line.
This is rather illogical and difficult to emulate properly.

ncw commented Sep 15, 2019

Copy link
Copy Markdown
Collaborator Author

OK I did that! PTAL

corona10 commented Sep 15, 2019

Copy link
Copy Markdown
Collaborator

@ncw

gpython/parser/lexer.go

Line 375 in 2609237

func (x *yyLex) Lex(yylval *yySymType) (ret int) {

Can we fix this issue on lexer level approach?

corona10 commented Sep 28, 2019

Copy link
Copy Markdown
Collaborator

@ncw
This PR is pending for 2 weeks.
Should we merge this PR for this time and later fix it?
Or do you have any ideas?
Thanks!

ncw commented Sep 28, 2019

Copy link
Copy Markdown
Collaborator Author

I think this is good to go now.

I don't think fixing it in the lexer is the right place - this is specifically to do with how the REPL works and in particular how the single input grammar works which is a bit strange but it is how the Python grammar is defined.

I'll merge this now and if there are any problem with it I'm sure we can fix them up :-)

ncw merged commit 8c361a8 into master Sep 28, 2019
corona10 deleted the fix-78-repl branch September 29, 2019 03:17
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

corona10 corona10 left review comments

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

Back | FazBrowse Home | New Git URL