Skip to content

parser: Update from go tool yacc into goyacc #22

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

Merged
merged 1 commit into from
Sep 3, 2018

Conversation

corona10
Copy link
Collaborator

@corona10 corona10 commented Sep 1, 2018

Since Go 1.8 removed the go tool yacc command,
we should update it to use goyacc

reference: https://beta.golang.org/doc/go1.8#tool_yacc

@corona10 corona10 requested review from ncw and sbinet September 1, 2018 16:17
@codecov-io
Copy link

codecov-io commented Sep 1, 2018

Codecov Report

Merging #22 into master will decrease coverage by 0.07%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #22      +/-   ##
==========================================
- Coverage    64.5%   64.43%   -0.08%     
==========================================
  Files          55       55              
  Lines        9740     9793      +53     
==========================================
+ Hits         6283     6310      +27     
- Misses       3010     3036      +26     
  Partials      447      447
Impacted Files Coverage Δ
parser/y.go 67.64% <ø> (-7.57%) ⬇️

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 ed3c651...cf2d78d. Read the comment docs.

@corona10
Copy link
Collaborator Author

corona10 commented Sep 1, 2018

@ncw @sbinet

PTAL

@corona10
Copy link
Collaborator Author

corona10 commented Sep 1, 2018

@ncw @sbinet
For reviewers, The code coverage results are not related with this CL.
We can ignore the result of this.

@corona10 corona10 force-pushed the goyacc branch 3 times, most recently from 228e63a to 9428ab9 Compare September 2, 2018 12:04
Since Go 1.8 removed the go tool yacc command,
we should update it to use goyacc
@corona10
Copy link
Collaborator Author

corona10 commented Sep 2, 2018

@ncw @sbinet
I notice that the code coverage metric is not exact.
I checked that the block which said "no-cover" is already covered on the unit test.

@sbinet
Copy link
Member

sbinet commented Sep 2, 2018

I am on a train + mobile, so I can't easily check... But is the piece of code you say should be tested, is tested within the same package? (There's probably a way to treat a whole repo like a single code-coverage unit so that parts of pkgA that are tested from pkgB still are counted as tested. But I haven't found it.)

@corona10
Copy link
Collaborator Author

corona10 commented Sep 2, 2018

@sbinet
Yes, they are on the same package "parser".

@ncw
Copy link
Collaborator

ncw commented Sep 3, 2018

I think that looks fine to me. I had a quick look through the auto generated code and it looks like it has just been tidied up. That may explain the code coverage oddities also since the line count of the generated code has increased.

Copy link
Collaborator

@ncw ncw left a comment

Choose a reason for hiding this comment

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

LGTM

@corona10 corona10 merged commit 6ded9bc into go-python:master Sep 3, 2018
@corona10 corona10 deleted the goyacc branch September 3, 2018 10:04
corona10 added a commit to corona10/gpython that referenced this pull request Nov 24, 2018
- Support auto-generated licence header based on created time.
- Add missed case when adding testcases go-python#22
corona10 added a commit to corona10/gpython that referenced this pull request Nov 24, 2018
- Support auto-generated license header based on created time.
- Add missed case when adding test cases go-python#22
ncw pushed a commit that referenced this pull request Nov 25, 2018
- Support auto-generated license header based on created time.
- Add missed case when adding test cases #22
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.

4 participants