Skip to content

#44 Display build information#52

Merged
corona10 merged 5 commits intogo-python:masterfrom
kislenko-artem:issue-44
Mar 3, 2019
Merged

#44 Display build information#52
corona10 merged 5 commits intogo-python:masterfrom
kislenko-artem:issue-44

Conversation

@kislenko-artem
Copy link
Contributor

@kislenko-artem kislenko-artem commented Jan 23, 2019 •
edited by corona10
Loading

Update RunREPL() to be able to display build information

Updates: #52

corona10
corona10 requested changes Jan 23, 2019
View reviewed changes
Copy link
Collaborator

@corona10 corona10 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)

@corona10 corona10 requested a review from ncw January 23, 2019 16:48
@kislenko-artem
Copy link
Contributor Author

kislenko-artem commented Jan 23, 2019 •
edited
Loading

@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

corona10 commented Jan 23, 2019

@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

corona10 commented Jan 23, 2019

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 •
edited
Loading

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 Files Coverage Δ
py/args.go 52.94% <0%> (-2.27%) ⬇️
py/call_iterator.go 75% <0%> (ø)
builtin/builtin.go 79.62% <0%> (+0.85%) ⬆️
py/type.go 51.22% <0%> (+1.63%) ⬆️
py/tuple.go 31.13% <0%> (+6.6%) ⬆️
py/internal.go 40.86% <0%> (+7.21%) ⬆️
py/list.go 21.3% <0%> (+8.28%) ⬆️
py/none.go 52.38% <0%> (+9.52%) ⬆️
py/dict.go 54.66% <0%> (+10.66%) ⬆️
py/sequence.go 34.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
Contributor Author

kislenko-artem commented Jan 24, 2019

@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

corona10
corona10 approved these changes Jan 24, 2019
View reviewed changes
Copy link
Collaborator

@corona10 corona10 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?

corona10
corona10 reviewed Jan 24, 2019
View reviewed changes
ncw
ncw requested changes Jan 27, 2019
View reviewed changes
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.

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
corona10 reviewed Jan 30, 2019
View reviewed changes
@corona10
Copy link
Collaborator

corona10 commented Feb 28, 2019

@kislenko-artem
Any progress on this PR?

cc @ncw

@kislenko-artem
Copy link
Contributor Author

kislenko-artem commented Mar 2, 2019 •
edited
Loading

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

@corona10
Copy link
Collaborator

corona10 commented Mar 2, 2019

@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

@corona10 corona10 self-requested a review March 3, 2019 01:33
corona10
corona10 requested changes Mar 3, 2019
View reviewed changes
corona10
corona10 approved these changes Mar 3, 2019
View reviewed changes
Copy link
Collaborator

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

corona10 commented Mar 3, 2019

@kislenko-artem

Thank you for your contribution!
Thanks a lot!

@corona10 corona10 merged commit 61059b4 into go-python:master Mar 3, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@ncw ncw ncw requested changes

@corona10 corona10 corona10 approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

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