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

SDK selection refactor #197

Merged
merged 18 commits into from
Apr 22, 2020
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
31 changes: 7 additions & 24 deletions config/config.go
Original file line number Diff line number Diff line change
Expand Up @@ -11,10 +11,9 @@ import (
"github.com/spf13/viper"
)

type ConfigWrapper struct {
type configWrapper struct {
Viper *viper.Viper
overridden map[string]interface{}
ReadDone bool
}

type DBConfig struct {
Expand All @@ -27,7 +26,7 @@ const lbrynetServers = "LbrynetServers"
const deprecatedLbrynet = "Lbrynet"

var once sync.Once
var Config *ConfigWrapper
var Config *configWrapper

// overriddenValues stores overridden v values
// and is initialized as an empty map in the read method
Expand All @@ -37,21 +36,21 @@ func init() {
Config = GetConfig()
}

func GetConfig() *ConfigWrapper {
func GetConfig() *configWrapper {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Returning variables of private types from public functions is not a recommended practice

Copy link
Contributor Author

Choose a reason for hiding this comment

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

can you link me to where this is not recommended? i see this in plenty of places and it makes sense to me

Copy link
Collaborator

@anbsky anbsky Apr 24, 2020

Choose a reason for hiding this comment

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

I learned this from golint and their reasoning seems compelling, namely:

can't put a *sample field in their own struct

Even in my not-so-long practice I distinctly remember being annoyed by this more than a few times.

once.Do(func() {
Config = NewConfig()
})
return Config
}

func NewConfig() *ConfigWrapper {
c := &ConfigWrapper{}
func NewConfig() *configWrapper {
c := &configWrapper{}
c.Init()
c.Read()
return c
}

func (c *ConfigWrapper) Init() {
func (c *configWrapper) Init() {
c.overridden = make(map[string]interface{})
c.Viper = viper.New()

Expand Down Expand Up @@ -80,12 +79,11 @@ func (c *ConfigWrapper) Init() {
c.Viper.AddConfigPath("$HOME/.lbrytv")
}

func (c *ConfigWrapper) Read() {
func (c *configWrapper) Read() {
err := c.Viper.ReadInConfig()
if err != nil {
panic(err)
}
c.ReadDone = true
}

// IsProduction is true if we are running in a production environment
Expand Down Expand Up @@ -193,21 +191,6 @@ func GetReflectorAddress() string {
return Config.Viper.GetString("ReflectorAddress")
}

// GetReflectorTimeout returns reflector TCP timeout in seconds.
func GetReflectorTimeout() int64 {
return Config.Viper.GetInt64("ReflectorTimeout")
}

// GetRefractorAddress returns refractor address in the format of host:port.
func GetRefractorAddress() string {
return Config.Viper.GetString("RefractorAddress")
}

// GetRefractorTimeout returns refractor TCP timeout in seconds.
func GetRefractorTimeout() int64 {
return Config.Viper.GetInt64("RefractorTimeout")
}

// ShouldLogResponses enables or disables full SDK responses logging
func ShouldLogResponses() bool {
return Config.Viper.GetBool("ShouldLogResponses")
Expand Down
25 changes: 25 additions & 0 deletions internal/test/test_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -43,3 +43,28 @@ func TestMockHTTPServer(t *testing.T) {
require.NoError(t, err)
assert.Equal(t, string(body), "ok")
}

func TestAssertJsonEqual(t *testing.T) {

testCases := []struct {
a, b string
same bool
}{
{"{}", "12", false},
{"{}", "{}", true},
{"{}", "", false},
{`{"a":1,"b":2}`, `{"b":2,"a":1}`, true},
}

for i, tc := range testCases {
testT := &testing.T{}
same := AssertJsonEqual(testT, tc.a, tc.b)
if tc.same {
assert.True(t, same, "Case %d same", i)
assert.False(t, testT.Failed(), "Case %d failure", i)
} else {
assert.False(t, same, "Case %d same", i)
assert.True(t, testT.Failed(), "Case %d failure", i)
}
}
}