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

Generate SyntaxError of global declaration by HyeockJinKim · Pull Request #74 · go-python/gpython · GitHub

/ gpython Public

Generate SyntaxError of global declaration - #74

Merged
corona10 merged 2 commits into
go-python:masterfrom
HyeockJinKim:issue72
Sep 15, 2019
ConversationCommits (2)ChecksFiles changed
Merged

Generate SyntaxError of global declaration#74
corona10 merged 2 commits into
go-python:masterfrom
HyeockJinKim:issue72

Conversation

HyeockJinKim commented Sep 10, 2019

Copy link
Copy Markdown
Contributor

Generate SyntaxError instead of SyntaxWarning
if global declaration is for priviously used variable

Fixes #72

Generate SyntaxError of global declaration
ba04200 
Generate SyntaxError instead of SyntaxWarning
if global declaration is for priviously used variable

Fixes go-python#72
corona10 self-requested a review September 10, 2019 14:08

corona10 commented Sep 10, 2019 •
edited
Loading

Copy link
Copy Markdown
Collaborator

@HyeockJinKim
Raising SyntaxError is the right solution but
It looks like tests should be updated

corona10 requested a review from ncw September 10, 2019 14:14

corona10 commented Sep 10, 2019

Copy link
Copy Markdown
Collaborator

@ncw
Would you like to guide about updating the unit test for him?
If his approach is not proper, feedback will be helpful to him

codecov-io commented Sep 11, 2019

Copy link
Copy Markdown

Codecov Report

Merging #74 into master will not change coverage.
The diff coverage is 100%.

@@           Coverage Diff           @@
##           master      #74   +/-   ##
=======================================
  Coverage   68.65%   68.65%           
=======================================
  Files          59       59           
  Lines       10525    10525           
=======================================
  Hits         7226     7226           
  Misses       2790     2790           
  Partials      509      509
Impacted Files Coverage Δ
symtable/symtable.go 94.23% <100%> (ø) ⬆️

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 7bc4005...93a13f1. Read the comment docs.

HyeockJinKim commented Sep 11, 2019

Copy link
Copy Markdown
Contributor Author

@corona10 Can I get a review?

corona10 reviewed Sep 11, 2019
View reviewed changes

corona10 left a comment

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

@HyeockJinKim
It looks working well but I am not sure this is the right way to deal with.
Let's wait @ncw 's review

ncw commented Sep 15, 2019

Copy link
Copy Markdown
Collaborator

I think the code looks nice :-)

However it makes gpython incompatible with python3.4 as I wrote here: #72 (comment)

Now I don't think any of the tests rely on that so it is safe to change it.

What do you think @corona10 - make a break with python3.4 compatibility here? On balance I think it is a good idea since this is almost always an actual problem.

PS the commits need squashing which can be done at merge time

corona10 commented Sep 15, 2019

Copy link
Copy Markdown
Collaborator

@ncw
I am not on +1 or -1 on breaking the 3.4 compatibilities.
However when we update the project objective into implementing Python3.x interpreter.
Breaking with python3.4 compatibility is not a bad idea if so that's a way we have to go.

ncw commented Sep 15, 2019

Copy link
Copy Markdown
Collaborator

@ncw
I am not on +1 or -1 on breaking the 3.4 compatibilities.
However when we update the project objective into implementing Python3.x interpreter.
Breaking with python3.4 compatibility is not a bad idea if so that's a way we have to go.

OK, lets squash and merge :-)

corona10 merged commit eb115a9 into go-python:master Sep 15, 2019

corona10 commented Sep 15, 2019

Copy link
Copy Markdown
Collaborator

@HyeockJinKim
Thanks for the contribution
and thanks for the review @ncw :)

corona10 mentioned this pull request Oct 14, 2019
Open
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
ncw Awaiting requested review from ncw

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

SyntaxError of global declaration don't stop program

4 participants

Back | FazBrowse Home | New Git URL