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

[WIP] Read env variables from file #5

Open
wants to merge 9 commits into
base: master
Choose a base branch
from

Conversation

ilya-korotya
Copy link

No description provided.

@codecov-io
Copy link

codecov-io commented Oct 7, 2019

Codecov Report

Merging #5 into master will decrease coverage by 2.02%.
The diff coverage is 50%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master       #5      +/-   ##
==========================================
- Coverage   57.94%   55.92%   -2.03%     
==========================================
  Files           8        8              
  Lines         302      304       +2     
==========================================
- Hits          175      170       -5     
- Misses        106      111       +5     
- Partials       21       23       +2
Impacted Files Coverage Δ
value.go 53.23% <50%> (-2.97%) ⬇️
priority.go 59.09% <0%> (-9.1%) ⬇️

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 a80acba...435d55c. Read the comment docs.

env.go Outdated
return err
}
if err == io.EOF && len(pair) == 0 {
return nil
Copy link
Owner

Choose a reason for hiding this comment

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

if err != nil {
if err == io.EOF && len(pair) == 0 {
return nil
}
return err
}

Copy link
Author

Choose a reason for hiding this comment

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

Here you made error (proebalis`)

env.go Outdated
ErrInvalidPair = errors.New("invalid pair for env variable")
)

type EnvConfig struct{}
Copy link
Owner

Choose a reason for hiding this comment

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

please add comment

env.go Outdated
return &EnvConfig{}
}

func (e *EnvConfig) SetEnv(data io.Reader) error {
Copy link
Owner

Choose a reason for hiding this comment

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

should be separated logic for parsing env file and setting env variables

env.go Outdated
ErrInvalidPair = errors.New("invalid pair for env variable")
)

type EnvConfig struct{}
Copy link
Owner

Choose a reason for hiding this comment

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

also would be nice if this struct implements external interface

env.go Outdated
Comment on lines 31 to 32
pair = bytes.Trim(pair, "\n")
i := bytes.Index(pair, []byte("="))
Copy link
Owner

Choose a reason for hiding this comment

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

read how to properly parse env file

env.go Outdated
)

var (
ErrInvalidPair = errors.New("invalid pair for env variable")
Copy link
Owner

Choose a reason for hiding this comment

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

comment

env.go Outdated
return nil
}
}
n = bytes.Trim(n, " ")
Copy link
Owner

Choose a reason for hiding this comment

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

simply use strings.TrimSpace

env.go Outdated
)

const (
commentSymbol = '#'
Copy link
Owner

Choose a reason for hiding this comment

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

can be defined in Parse func scope

Copy link
Author

Choose a reason for hiding this comment

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

@antonmashko I think this comment relates more to the style of the code.

Copy link
Owner

Choose a reason for hiding this comment

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

yep, it is

env.go Outdated
if i == -1 {
return ErrInvalidPair
}
key := string(n[:i])
Copy link
Owner

Choose a reason for hiding this comment

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

  1. key and value also should be trimmed on white spaces
  2. strings.Trim(value, "`"")

Copy link
Author

Choose a reason for hiding this comment

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

@antonmashko

  1. We must enable the user to specify a space at the beginning/end of the line/key;
  2. ``` this is invalid chapter for env variables.

Copy link
Owner

Choose a reason for hiding this comment

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

  1. What do you mean? If you try in bash define variable like FOO=" BAR" and print FOO variable, result will be BAR without spaces.
  2. Sorry, my fault. I meant ' character instead of `

@@ -186,15 +186,16 @@ func (v *value) define() error {
owner = owner.parent
}
value, exists = v.owner.external.Get(values...)
if exists {
Copy link
Owner

Choose a reason for hiding this comment

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

this changes is not related to this commit

env.go Outdated
if i == -1 {
return ErrInvalidPair
}
key := string(n[:i])
Copy link
Owner

Choose a reason for hiding this comment

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

  1. What do you mean? If you try in bash define variable like FOO=" BAR" and print FOO variable, result will be BAR without spaces.
  2. Sorry, my fault. I meant ' character instead of `

@ilya-korotya ilya-korotya changed the title Read env variables from file [WIP] Read env variables from file Oct 24, 2019
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.

4 participants