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

Fix base currency option handling #2063

Merged
merged 1 commit into from
Dec 5, 2018

Conversation

cbeams
Copy link
Contributor

@cbeams cbeams commented Dec 5, 2018

This change fixes #2048 by removing the assignment of a default value
for the baseCurrencyNetwork option at the level of the command line
option parser. The assignment of this default was an oversight in #1961
(specifically commit 83e1dd3) that did not account for the fact that
users can change the baseCurrencyNetwork value via the Settings screen
in the application. When users change the setting in the application, the new
value is persisted to /bisq.properties, which is handled at
runtime as a PropertySource with lower precedence than the command line
property source, which means that the changed value is never picked up
because the higher-precedence command line PropertySource always has a
default value.

This fix is surgical in that it addresses only this specific option. A
subsequent change should address the more general issue that setting
defaults in the command line option parser always precludes the
possibility of overriding them in bisq.properties. Basically, we should
revert to the previous strategy of reporting what the default value will
be in the help text without actually assigning a default value in the
option parser using the defaultsTo method.

This change fixes bisq-network#2048 by removing the assignment of a default value
for the `baseCurrencyNetwork` option at the level of the command line
option parser. The assignment of this default was an oversight in bisq-network#1961
(specifically commit 83e1dd3) that did not account for the fact that
users can change the `baseCurrencyNetwork` value via the Settings screen
in the application. When users change the setting in the application, the new
value is persisted to <appDataDir>/bisq.properties, which is handled at
runtime as a PropertySource with lower precedence than the command line
property source, which means that the changed value is never picked up
because the higher-precedence command line PropertySource always has a
default value.

This fix is surgical in that it addresses only this specific option. A
subsequent change should address the more general issue that setting
defaults in the command line option parser always precludes the
possibility of overriding them in bisq.properties. Basically, we should
revert to the previous strategy of reporting what the default value will
be in the help text without actually assigning a default value in the
option parser using the `defaultsTo` method.
Copy link
Contributor

@ManfredKarrer ManfredKarrer left a comment

Choose a reason for hiding this comment

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

utACK

@ManfredKarrer
Copy link
Contributor

Thanks @cbeams for fixing it so fast! @freimair I merge it already so that users who run from source can this fixed. But please have a look if we need to change that for the other options as well. At least with the path for log files there was an issue as well (which is fixed by not using getProperty).

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.

Changing BTC network does not have any effect
2 participants