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

Read config #55

Merged
merged 4 commits into from
Jul 25, 2018
Merged

Read config #55

merged 4 commits into from
Jul 25, 2018

Conversation

smacker
Copy link
Contributor

@smacker smacker commented Jul 24, 2018

Part of #20

There is no merging (yet?). Local configuration of analyzer just replaces the global one.

Signed-off-by: Maxim Sukharev <maxim@sourced.tech>
Signed-off-by: Maxim Sukharev <maxim@sourced.tech>
Signed-off-by: Maxim Sukharev <maxim@sourced.tech>
@smacker smacker requested review from carlosms and bzz July 24, 2018 15:09
Copy link
Contributor

@carlosms carlosms left a comment

Choose a reason for hiding this comment

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

👍

server.go Outdated
}

// Config is a server configuration
type Config struct {
Copy link
Contributor

Choose a reason for hiding this comment

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

a name like ServerConfig may be could help avoid the confusion?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

renamed

@bzz bzz mentioned this pull request Jul 25, 2018
5 tasks
)

// ToStruct converts a map[string]interface{} to a types.Struct
func ToStruct(v map[string]interface{}) *types.Struct {
Copy link
Contributor

Choose a reason for hiding this comment

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

I did not use gogo protobuf very much, so could you please share a bit, why in order to create new Struct a https://godoc.org/github.com/gogo/protobuf/types#NewPopulatedStruct would not work for us?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Because it accepts randyStruct but we need map[string]interface{}.
We parse unknown structure from yaml, the only type we can use is map[string]interface{}.

@bzz
Copy link
Contributor

bzz commented Jul 25, 2018

Overall SGTM, but a bit confused with how instances of type.Struct are created.

@smacker
Copy link
Contributor Author

smacker commented Jul 25, 2018

@bzz please suggest any other way to create Struct if you know any.

Signed-off-by: Maxim Sukharev <maxim@sourced.tech>
@bzz
Copy link
Contributor

bzz commented Jul 25, 2018

I do not, but my hope here was that may be you, as an author, have already done some kind of research and could share the results i.e if some gogo/protobuf extensions might be able to provide such thing.

If not - it is fine just to say that you did not.

I.e one helpful feedback would include pointing to the fact that official Golang protobuf impl is not going to provide this golang/protobuf#370 and clients are expected to implement the conversion \w reflections and type assertions, as you did here.

@smacker
Copy link
Contributor Author

smacker commented Jul 25, 2018

Of course, I have done research. I found nothing better than creating a custom transformer. So I had to implement it by myself. You see it in PR.

Your point is it looks "confused". Sadly, I don't know how to make it less confusing.
Please provide your suggestion on how to fix it. Otherwise, the discussion isn't very productive.

@smacker smacker merged commit 0ac15b5 into src-d:master Jul 25, 2018
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.

3 participants