Skip to content
This repository has been archived by the owner on Dec 7, 2023. It is now read-only.

Fill default settings #584

Merged
merged 4 commits into from
May 12, 2020
Merged

Fill default settings #584

merged 4 commits into from
May 12, 2020

Conversation

superboyiii
Copy link
Member

Close #583

@shargon
Copy link
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
Member Author

@shargon Change for what?

@shargon
Copy link
Member

shargon commented May 12, 2020

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

@superboyiii
Copy link
Member Author

@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
@@ -52,7 +52,7 @@ public class StorageSettings

public StorageSettings(IConfigurationSection section)
{
this.Engine = section.GetSection("Engine").Value;
this.Engine = section.GetValue("Engine", "LevelDBStore");
Copy link
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
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
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
Member Author

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
Member

shargon commented May 12, 2020

dotnet format please

@superboyiii
Copy link
Member Author

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

@shargon
Copy link
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 subscribe to this conversation on GitHub. Already have an account? Sign in.
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