Skip to content
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

Add cmd/goversioninfo command-line app #1

Merged
merged 8 commits into from
Jan 22, 2015
Merged

Add cmd/goversioninfo command-line app #1

merged 8 commits into from
Jan 22, 2015

Conversation

tgulacsi
Copy link

Hi,

I've just copied your example to a command-line app,
plus made some functions return error, and check that error.

Plus cosmetic changes (go vet + golint), except that goling says we should not use names with underscores - but that's subjective, and I don't want to be rude.

The remaining big question is how should we fill the versioninfo.json, if not edited by hand?
I've thought the command-line app should get flags for each field in StringFileInfo struct, and overload that.
But I don't know whether this would be enough, or useful at all.

What's your opinion?

@coveralls
Copy link

Coverage Status

Coverage increased (+0.23%) when pulling 242904a on tgulacsi:master into eb3ce99 on josephspurrier:master.

@josephspurrier
Copy link
Owner

You are the man! Awesome changes. I like the cmd folder addition.

I'll change the names so they don't have underscores. I copied the VS_ from the Microsoft Spec then then followed that convention for all structs, but I've rather follow Go conventions then Microsoft considering the VS_VERSIONINFO is not true C-language structure.

For versioninfo.json:
According to the Microsoft spec, the CompanyName/FileVersion strings are just suggestions so it should make sense to allow for a more flexible structure - adding the parse flags would not be a good solution. The VS_StringFileInfo.children is also supposed to be an array - I left that out since this is the first version. If you look at an MSI, there can be multiple sections in the file properties.

I need to think about that. Suggestions?

@coveralls
Copy link

Coverage Status

Coverage decreased (-1.14%) when pulling 5f4ea05 on tgulacsi:master into eb3ce99 on josephspurrier:master.

@coveralls
Copy link

Coverage Status

Coverage decreased (-1.14%) when pulling 15dc879 on tgulacsi:master into eb3ce99 on josephspurrier:master.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.5%) when pulling 201d329 on tgulacsi:master into eb3ce99 on josephspurrier:master.

@coveralls
Copy link

Coverage Status

Coverage increased (+1.47%) when pulling 8b4a647 on tgulacsi:master into eb3ce99 on josephspurrier:master.

@tgulacsi
Copy link
Author

I have no power left, so I leave the changes in README to another day / you.
A possible use case (with go generate) is at tgulacsi/agostle@57ede16

@josephspurrier
Copy link
Owner

Amazing work! I'm going to go through this tonight!

josephspurrier added a commit that referenced this pull request Jan 22, 2015
Add cmd/goversioninfo command-line app, separate out code, added constants for all the langid and charsetid, increased test coverage, fixed code using vet and lint
@josephspurrier josephspurrier merged commit a8bab91 into josephspurrier:master Jan 22, 2015
@josephspurrier
Copy link
Owner

I made the additional changes we discussed. Let me know if there is anything else you can think of.

@josephspurrier
Copy link
Owner

Also, how come you don't show up as a contributor? Did I merge it correctly?

@tgulacsi
Copy link
Author

Hi,

I think you made a great job rewriting the README, and adding some needed
flags to the command-line app.
I don't know who will become a "Contributor" in GitHub's terms, but it is
not a big thing - I know what I did, and you know, and Git guards it :)

2015-01-24 14:48 GMT+01:00 Joseph Spurrier notifications@github.com:

Also, how come you don't show up as a contributor? Did I merge it
correctly?


Reply to this email directly or view it on GitHub
#1 (comment)
.

@josephspurrier
Copy link
Owner

Well, regardless, I appreciate all your help. I learned a lot from your code and I appreciate you taking the time to make the package awesome! Thanks!!

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.

None yet

3 participants