Skip to content

Conversation

@ncw
Copy link
Collaborator

@ncwncw commented Sep 15, 2019

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
Copy link
CollaboratorAuthor

ncw commented Sep 15, 2019

Can you review this for me @corona10 ?

@codecov-io
Copy link

codecov-io commented Sep 15, 2019

Codecov Report

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

Impacted file tree graph

@@ 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 FilesCoverage Δ
repl/repl.go100% <100%> (ø)⬆️
symtable/symtable.go94.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.

Copy link
Collaborator

@corona10corona10 left a comment

Choose a reason for hiding this comment

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

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?

[Gpythondev] -os/arch: darwin/amd64-goversion: go1.10>>>defa(): pass>>>

@corona10
Copy link
Collaborator

This is the example of pypy and rustpython.

Python3.6.1 (dab365a465140aa79a5f3ba4db784c4af4d5c195, Feb182019, 10:53:27) [PyPy7.0.0-alpha0withGCC4.2.1CompatibleAppleLLVM10.0.0 (clang-1000.11.45.5)] ondarwinType"help", "copyright", "credits"or"license"formoreinformation. Andnowforsomethingcompletelydifferent: ``"it'slikelytemporaryuntilforever" arigo''>>>>#>>>>#>>>>
WelcometothemagnificentRustPython0.1.0interpreter 😱 🖖 >>>>>#>>>>>

@corona10
Copy link
Collaborator

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

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

@ncw
Copy link
CollaboratorAuthor

ncw commented Sep 15, 2019

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

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
Copy link
Collaborator

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

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
Copy link
CollaboratorAuthor

ncw commented Sep 15, 2019

OK I did that! PTAL

@corona10
Copy link
Collaborator

@ncw

func (x*yyLex) Lex(yylval*yySymType) (retint){

Can we fix this issue on lexer level approach?

@corona10
Copy link
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
Copy link
CollaboratorAuthor

ncw commented Sep 28, 2019

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 :-)

@ncwncw merged commit 8c361a8 into masterSep 28, 2019
@corona10corona10 deleted the fix-78-repl branch September 29, 2019 03:17
Sign up for freeto 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.

4 participants

@ncw@codecov-io@corona10