The legend of xDays is back! #108
Conversation
I have added xDaysThreshold I don't know what I did, but this "might" work.
|
LGTM! 👍 |
|
The change in logic is ok, however, documentation and backward compatibility are not taken care of. |
|
I will try to fix backward compatibility. Is it ok if I just check if sixtydaythreshold is still present and if it is, use that instead of xdaythreshold? |
|
Yeah that would be perfect, just don't want to break existing configs. If sixtydaythreshold is present use it's value for the xdaythreshold with xdays=60? |
|
I have tried to fix Backwards compatibility on my own branch. I will not put it on this pull request before I have tested it for bugs. |
|
I have tried to make it backwards compatible. It works on my local copy, but try to see if it works. |
rnevet
left a comment
rnevet
left a comment
There was a problem hiding this comment.
Looks good, the README description should also be updated to x days.
Currently I can't test the changes myself.
|
Done. |
lendingbot.py
Outdated
|
|
||
| #Check if we need a config file at all (If all settings are passed by args, we won't) | ||
| if args.apikey and args.apisecret and args.sleeptimeactive and args.sleeptimeinactive and args.mindailyrate and args.maxdailyrate and args.spreadlend and args.gapbottom and args.gaptop and args.sixtydaythreshold: | ||
| if args.apikey and args.apisecret and args.sleeptimeactive and args.sleeptimeinactive and args.mindailyrate and args.maxdailyrate and args.spreadlend and args.gapbottom and args.gaptop and args.xdaythreshold and args.xdays: |
There was a problem hiding this comment.
This means that sixtydaythreshold will not work as command line?
There was a problem hiding this comment.
I didn't know what to put there, so I just changed it from sixtydaythreshold to xdays...
There was a problem hiding this comment.
I would say something like:
... and ((args.xdaythreshold and args.xdays) or args.sixtydaythreshold)
|
@Evanito, can you test this a bit before we merge it? I won't have access to my dev environment in the upcoming weeks. |
|
It was running when I started the pull request. I have 0 problems related to my changes. I have also updated the bot to my newest changes. |
|
That's great, I would like to have 1 more person running it before merging. I would myself but can't at the moment. |
Evanito
left a comment
Evanito
left a comment
There was a problem hiding this comment.
Code looks good to me logically.
3 participants
I have created a new version of my feature xDays. I have used this configuration for about a week. I copied the changes made to Github.
I'm not quite sure what I did. The script has evolved a bit since last time I used it. I just tried and tried until it was stable. I havn't observed any issues with it yet.
Feel free to look at it and see if it fits.
Old pull request: #8