FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

#44 Display build information by kislenko-artem · Pull Request #52 · go-python/gpython · GitHub

/ gpython Public

#44 Display build information - #52

Merged
corona10 merged 5 commits into
go-python:masterfrom
kislenko-artem:issue-44
Mar 3, 2019
ConversationCommits (5)ChecksFiles changed
Merged

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

Conversation

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

Copy link
Copy Markdown
Contributor

Update RunREPL() to be able to display build information

Updates: #52

corona10 requested changes Jan 23, 2019
View reviewed changes

corona10 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

corona10 requested a review from ncw January 23, 2019 16:48

kislenko-artem commented Jan 23, 2019 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

@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 commented Jan 23, 2019

Copy link
Copy Markdown
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 commented Jan 23, 2019

Copy link
Copy Markdown
Collaborator

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

Update RunREPL() to display build information

Updates: #52

codecov-io commented Jan 24, 2019 •
edited
Loading

Copy link
Copy Markdown

Codecov Report

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

@@            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 commented Jan 24, 2019

Copy link
Copy Markdown
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:

corona10 approved these changes Jan 24, 2019
View reviewed changes

corona10 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

@ncw Do you have any ideas?

corona10 reviewed Jan 24, 2019
View reviewed changes
Comment thread version.go
ncw requested changes Jan 27, 2019
View reviewed changes

ncw left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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 reviewed Jan 30, 2019
View reviewed changes
Comment thread main.go Outdated

corona10 commented Feb 28, 2019

Copy link
Copy Markdown
Collaborator

@kislenko-artem
Any progress on this PR?

cc @ncw

kislenko-artem commented Mar 2, 2019 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

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

corona10 commented Mar 2, 2019

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

corona10 self-requested a review March 3, 2019 01:33
corona10 requested changes Mar 3, 2019
View reviewed changes
Comment thread repl/cli/cli.go Outdated
corona10 approved these changes Mar 3, 2019
View reviewed changes

corona10 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

LGTM

corona10 commented Mar 3, 2019

Copy link
Copy Markdown
Collaborator

@kislenko-artem

Thank you for your contribution!
Thanks a lot!

corona10 merged commit 61059b4 into go-python:master Mar 3, 2019
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

ncw ncw requested changes
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

Back | FazBrowse Home | New Git URL