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

Created algocfg to provide cli access to config.json #237

Merged
merged 3 commits into from Aug 14, 2019

Conversation

@Karmastic
Copy link
Contributor

Karmastic commented Aug 13, 2019

Adds algocfg get|set|reset commands to manipulate config.json
in a consistent and typesafe way.
Modified config.json is persisted minimally - only non-default
values are written. Versioning is taken into account and migration
is performed as required.

This also includes a fix to SaveNonDefaultValuesToFile() which was
writing config.json files with a trailing empty line for each default
value that was skipped.

Karmastic added 2 commits Aug 13, 2019
Adds `algocfg get|set|reset` commands to manipulate config.json
in a consistent and typesafe way.
Modified config.json is persisted minimally - only non-default
values are written.  Versioning is taken into account and migration
is performed as required.

This also includes a fix to SaveNonDefaultValuesToFile() which was
writing config.json files with a trailing empty line for each default
value that was skipped.
@Karmastic Karmastic requested review from algobolson and tsachiherman Aug 13, 2019

var dataDirs []string

func resolveDataDir() string {

This comment has been minimized.

Copy link
@algobolson

algobolson Aug 14, 2019

Collaborator

these functions exist elsewhere (cmd/goal/... at least) and maybe it's time to extract them? If not libgoal than some other library of algo-command-utils

This comment has been minimized.

Copy link
@Karmastic

Karmastic Aug 14, 2019

Author Contributor

Yes they do. I moved in the direction of extracting them in this PR - moving them to distinct files. I didn't want to incur the extra cost right now.

func setFieldValue(field reflect.Value, value string) error {
switch k := field.Kind(); k {
case reflect.Int, reflect.Int8, reflect.Int16, reflect.Int32, reflect.Int64:
val, err := strconv.ParseInt(value, 10, 64)

This comment has been minimized.

Copy link
@algobolson

algobolson Aug 14, 2019

Collaborator

we could ParseInt(value, 0, 64), 0-base allows for detecting 0777 octal or 0xabc hex. (Also ParseUint below)

This comment has been minimized.

Copy link
@Karmastic

Karmastic Aug 14, 2019

Author Contributor

We could, but we do not want to or need to. I don't want it to be ambiguous.

Copy link
Contributor

tsachiherman left a comment

looks good.

@Karmastic Karmastic requested a review from algobolson Aug 14, 2019
@Karmastic Karmastic merged commit c47849a into algorand:master Aug 14, 2019
2 checks passed
2 checks passed
Travis CI - Pull Request Build Passed
Details
license/cla Contributor License Agreement is signed.
Details
@Karmastic Karmastic deleted the Karmastic:algocfg branch Aug 14, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.