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

Proposal: change in configuration settings. #192

Open
gasteazi opened this issue May 15, 2021 · 6 comments
Open

Proposal: change in configuration settings. #192

gasteazi opened this issue May 15, 2021 · 6 comments
Labels
enhancement New feature or request feature request Feature request for roadmap help required Help required from a contributor

Comments

@gasteazi
Copy link

Hi, I would like to propose a change in the way custom settings are configured.
Currently it is very unclear to know which settings are enabled and which are not since sometimes setting 0 enables a functionality and sometimes disables it.
I propose that the custom configuration settings should always be 0 Disable and 1 Enable. This way there will be no confusion.

I take this opportunity to congratulate the great work you are doing.

@rcarmo
Copy link
Collaborator

rcarmo commented May 15, 2021

I agree with this. Since there are no other issues regarding config right now, I'd like to add that using click to handle CLI arguments would help make things consistent in that way.

@patrickfitz
Copy link

patrickfitz commented May 18, 2021

There is a super simple solution to this. In your config, swap 0 for false, and 1 for true.
No quotes, all lower case. Problem should be solved - at least it was for my last pull which was a week ago.
Way more readable too.

Though I agree that some of options are confusingly named - leading to a double-negatives (disablebullonly = false means....I've enabled what?)

@whittlem
Copy link
Owner

I am open for suggestions. I just don't want to break existing bots when updated without updating the configs. Maybe if we can detect and migrate the settings automatically for the users this would be good. What do you think?

@patrickfitz
Copy link

So I think you should Introduce versioning. Say we are on version 0.1 now, introduce the extra parameters at the next release - v0.2 - then commence deprecating those older parameters by issuing warnings in the logs when they are used.
Then finally remove them say 3 versions later.
I think that whilst rewriting the configs after migrating them might be a bit weird, unless you write the new file as something like "config.json.new", and send a message saying "there are new settings that you can find at" but really, I reckon verisoning is a better idea...

@whittlem whittlem added enhancement New feature or request feature request Feature request for roadmap labels May 23, 2021
@yanone
Copy link

yanone commented May 28, 2021

I propose that the custom configuration settings should always be 0 Disable and 1 Enable. This way there will be no confusion.

This issue is very dear to me, too, and I was about to open a new one on it.

The current situation of negative setting names that need to be negated to be activated or deactivated is unfortunate. It inevitably leads to erroneous usage. Has definitely already happened to me, and I'm a software developer. I don't have the ability to cleanly resolve double-negative settings in my brain, and even less so for even less experienced users.

The solution is simple: Rename all negative settings to positive, and turn around their defaults. Turn disablebuynearhigh=1 into buynearhigh=0 and nobody has an understanding problem.

You can keep parsing the old setting names while simply adding and publicly communicating only the new ones without breaking compatibility. You already use a method for querying each setting in your code, so you don't even need to touch your code generally, only the setting implementation.

You should raise an error if both old and new setting names are present in the config. It's only a few settings that are affected.

@whittlem
Copy link
Owner

whittlem commented Jun 7, 2021

Any volunteers to help me with this?

@whittlem whittlem added the help required Help required from a contributor label Jun 7, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request feature request Feature request for roadmap help required Help required from a contributor
Projects
None yet
Development

No branches or pull requests

5 participants