Skip to content

Fill default settings#584

Merged
superboyiii merged 4 commits intomasterfrom
fill-default-settings
May 12, 2020
Merged

Fill default settings#584
superboyiii merged 4 commits intomasterfrom
fill-default-settings

Conversation

@superboyiii
Copy link
Copy Markdown
Member

@superboyiii superboyiii commented May 12, 2020

Close #583

@shargon
Copy link
Copy Markdown
Member

shargon commented May 12, 2020 •
edited
Loading

Could you change also?

https://github.com/neo-project/neo-node/pull/584/files#diff-0327b59f4dd09986f49db7500602d9a9R88-R91

@superboyiii superboyiii mentioned this pull request May 12, 2020
Merged
@superboyiii
Copy link
Copy Markdown
Member Author

superboyiii commented May 12, 2020

@shargon Change for what?

@shargon
Copy link
Copy Markdown
Member

shargon commented May 12, 2020

Add the default value, if the section exists, but the values don't

@superboyiii
Copy link
Copy Markdown
Member Author

superboyiii commented May 12, 2020

@shargon I've already add the default values in this PR :) Or which param else do you think is need to have default value as well?

erikzhang
erikzhang reviewed May 12, 2020
View reviewed changes
neo-cli/Settings.cs
public StorageSettings(IConfigurationSection section)
{
this.Engine = section.GetSection("Engine").Value;
this.Engine = section.GetValue("Engine", "LevelDBStore");
Copy link
Copy Markdown
Member

@erikzhang erikzhang May 12, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

The default value should be MemoryStore. Because LevelDBStore might be not installed.

Copy link
Copy Markdown
Member Author

@superboyiii superboyiii May 12, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

@erikzhang But due to config.json's default settings, it's using LevelDBStore but not MemoryStore, or shall I modify config.json as well?

neo-node/neo-cli/config.json

Line 4 in 605fccb

"Engine": "LevelDBStore"

@shargon
Copy link
Copy Markdown
Member

shargon commented May 12, 2020

I've already add the default values in this PR :) Or which param else do you think is need to have default value as well?

Lines 88-91 https://github.com/neo-project/neo-node/pull/584/files#diff-0327b59f4dd09986f49db7500602d9a9R88-R91

@superboyiii
Copy link
Copy Markdown
Member Author

superboyiii commented May 12, 2020

I've already add the default values in this PR :) Or which param else do you think is need to have default value as well?

Lines 88-91 https://github.com/neo-project/neo-node/pull/584/files#diff-0327b59f4dd09986f49db7500602d9a9R88-R91

The initial value of public bool IsActive { get; } is always false. So that it will not influence cli running if the params of UnlockWalletSettings is null.
image
But add default value is more safe. I'll add it.

@shargon
Copy link
Copy Markdown
Member

shargon commented May 12, 2020

dotnet format please

@superboyiii
Copy link
Copy Markdown
Member Author

superboyiii commented May 12, 2020

@shargon Could you fix this? I don't know much about it.
image

@shargon
Copy link
Copy Markdown
Member

shargon commented May 12, 2020

Could you fix this? I don't know much about it.

Done, re-run and it works

@superboyiii superboyiii merged commit 2aadff1 into master May 12, 2020
@superboyiii superboyiii deleted the fill-default-settings branch May 12, 2020 17:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@erikzhang erikzhang erikzhang left review comments

@shargon shargon shargon approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Some params don't have default values in settings

3 participants

@superboyiii @shargon @erikzhang