Skip to content

#44 Display build information #52

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 5 commits into from
Mar 3, 2019
Merged

#44 Display build information #52

merged 5 commits into from
Mar 3, 2019

Conversation

kislenko-artem
Copy link
Contributor

@kislenko-artem kislenko-artem commented Jan 23, 2019

Update RunREPL() to be able to display build information

Updates: #52

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

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

@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

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

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
Copy link
Collaborator

@kislenko-artem
Any progress on this PR?

cc @ncw

@kislenko-artem
Copy link
Contributor Author

kislenko-artem commented Mar 2, 2019

@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
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
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants