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

#2 Add binary analysis #3

Open
wants to merge 12 commits into
base: main
Choose a base branch
from

Conversation

CatalinStratu
Copy link
Collaborator

No description provided.

Copy link
Member

@pombredanne pombredanne left a comment

Choose a reason for hiding this comment

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

Thanks!
Rather than an opaque 17MB testfile.exe, can you instead commit some source code that will be compiled to an exe during the tests, possibly with separate script.
We need a test too.

Also can you commit a proper go.mod and not only a go.sum?

analysis.go Outdated Show resolved Hide resolved
analysis.go Outdated Show resolved Hide resolved
analysis.go Outdated Show resolved Hide resolved
analysis.go Outdated Show resolved Hide resolved
analysis.go Outdated Show resolved Hide resolved
Copy link
Member

@pombredanne pombredanne left a comment

Choose a reason for hiding this comment

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

Thanks!
Can you add a CHANGELOG and add your name to the authors list?

examples/files/basic_golang_app/go.mod Show resolved Hide resolved
@CatalinStratu
Copy link
Collaborator Author

@pombredanne now we managed to change Gore with GoReSym. Now I am waiting for the code review.

Copy link
Member

@pombredanne pombredanne left a comment

Choose a reason for hiding this comment

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

Thanks! we need to setup tests, automated tests. And build too.

examples/files/basic_golang_app/go.mod Show resolved Hide resolved
examples/files/basic_golang_app/main.go Show resolved Hide resolved
analysis.go Outdated Show resolved Hide resolved
Copy link
Member

@pombredanne pombredanne left a comment

Choose a reason for hiding this comment

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

Thanks! Can you avoid committing a binary? Instead build it as part of the test suite run.

Copy link
Member

@pombredanne pombredanne 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 you need to add some Makefile or script to build things correctly and run tests locally too.
This should take care of building a test exe.... that should not be committed.

go-version: '1.20'

- name: Build Golang library
run: go build -v ./...
Copy link
Member

Choose a reason for hiding this comment

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

Why not run just go build -v?

@@ -33,5 +33,16 @@ jobs:
- name: Check for documentation style errors
working-directory: ./docs
run: ./scripts/doc8_style_check.sh

- name: Set up Go
Copy link
Member

Choose a reason for hiding this comment

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

You cannot run the tests as part of the documentation workflow. Either create a new workflow or use the azure pipeline instead.

analysis_test.go Show resolved Hide resolved
analysis_test.go Show resolved Hide resolved
@@ -0,0 +1,33 @@
//
Copy link
Member

Choose a reason for hiding this comment

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

Can you keep your package names all lower case? Use underscore as separator

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Can you give me a little more details?

examples/InspectLibraries/main.go Show resolved Hide resolved
examples/files/basic_golang_app/main.go Show resolved Hide resolved
examples/InspectLibraries/main.go Show resolved Hide resolved
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.

2 participants