Skip to content

Conversation

@kislenko-artem
Copy link
Contributor

@kislenko-artemkislenko-artem commented Jan 23, 2019

Update RunREPL() to be able to display build information

Updates: #52

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.

IMHO, we don't want to use makefile for this issue.
Please follow go-releaser way.
ref: #44 (comment)

@corona10corona10 requested a review from ncwJanuary 23, 2019 16:48
@kislenko-artem
Copy link
ContributorAuthor

kislenko-artem commented Jan 23, 2019

@corona10 ok. But how will you fill these variables? go build will work anyway. if compilation will run not via makefile, variables will retain default value.

@corona10
Copy link
Collaborator

@kislenko-artem
Please read the documentation which is linked on issue #52
https://goreleaser.com/environment/#using-the-main-version
I am not a go-releaser expert but on the documentation they say:

Default wise GoReleaser sets three ldflags: main.version: Current Git tag (the v prefix is stripped) or the name of the snapshot, if you’re using the --snapshot flag main.commit: Current git commit SHA main.date: Date according RFC3339 

@corona10
Copy link
Collaborator

Please fill the PR description if you possible.
e.g

Update RunREPL() to display build information Updates: #52 

@codecov-io
Copy link

codecov-io commented Jan 24, 2019

Codecov Report

Merging #52 into master will increase coverage by 1.91%.
The diff coverage is n/a.

Impacted file tree graph

@@ Coverage Diff @@## master #52 +/- ## ========================================== + Coverage 66.02% 67.94% +1.91%  ========================================== Files 58 59 +1 Lines 10246 10378 +132 ========================================== + Hits 6765 7051 +286 + Misses 3005 2828 -177 - Partials 476 499 +23
Impacted FilesCoverage Δ
py/args.go52.94% <0%> (-2.27%)⬇️
py/call_iterator.go75% <0%> (ø)
builtin/builtin.go79.62% <0%> (+0.85%)⬆️
py/type.go51.22% <0%> (+1.63%)⬆️
py/tuple.go31.13% <0%> (+6.6%)⬆️
py/internal.go40.86% <0%> (+7.21%)⬆️
py/list.go21.3% <0%> (+8.28%)⬆️
py/none.go52.38% <0%> (+9.52%)⬆️
py/dict.go54.66% <0%> (+10.66%)⬆️
py/sequence.go34.4% <0%> (+10.75%)⬆️
... and 5 more

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 95617b7...86379de. Read the comment docs.

@kislenko-artem
Copy link
ContributorAuthor

@corona10 unfortunately I can not fix description, I can edit only title. I removed makefile, sorry for my carelessness, I missed that project build via goreleaser

Now it is looked like this:
selection_845

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.

LGTM to me except we have to add add a paramter to RunREPL.

@ncw Do you have any ideas?

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.

I think this is looking very good :-)

However version.go is missing it's copyright header I think.

Once that is fixed up then we can merge :-)

@corona10
Copy link
Collaborator

@kislenko-artem
Any progress on this PR?

cc @ncw

@kislenko-artem
Copy link
ContributorAuthor

kislenko-artem commented Mar 2, 2019

@corona10 very very sorry, I thought my participation was ended. I added copyright information.

@corona10
Copy link
Collaborator

@kislenko-artem

Almost done,
Please follow the comment I left.
https://github.com/go-python/gpython/pull/52/files#r252264657

We don't want to pass paramters to RunREPL for the version information

@corona10corona10 self-requested a review March 3, 2019 01:33
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.

LGTM

@corona10
Copy link
Collaborator

@kislenko-artem

Thank you for your contribution!
Thanks a lot!

@corona10corona10 merged commit 61059b4 into go-python:masterMar 3, 2019
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

@kislenko-artem@corona10@codecov-io@ncw