Skip to content

Conversation

@corona10
Copy link
Collaborator

@corona10corona10 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

@corona10corona10 requested review from ncw and sbinetSeptember 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 FilesCoverage Δ
parser/y.go67.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
CollaboratorAuthor

@ncw@sbinet

PTAL

@corona10
Copy link
CollaboratorAuthor

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.

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

@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

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
CollaboratorAuthor

@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.

ncw
ncw approved these changes Sep 3, 2018
Copy link
Collaborator

@ncwncw left a comment

Choose a reason for hiding this comment

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

LGTM

@corona10corona10 merged commit 6ded9bc into go-python:masterSep 3, 2018
@corona10corona10 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 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

@corona10@codecov-io@sbinet@ncw