-
-
Notifications
You must be signed in to change notification settings - Fork 403
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
cli: look harder for $PWD/cfg, don't automatically start wizard #2299
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't check all the docstring, but I believe it would be better to check more of them. This is a clear behavior change, one that I agree with, but I really don't like that docstrings and/or documentation have not been updated accordingly. This is user facing, so it is of the upmost importance.
32a9fd9
to
5e65741
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a nitpick. Looks good to me otherwise.
I added the label breaking change because it changes the behavior of an existing feature that was kept as is between Sopel 6.9 and 7.1. I think it's an acceptable change that also need a fair warning to our users in the release note, without needing a whole deprecation cycle. |
1c0a10c
to
78edd0e
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same nitpick as dgw regarding the blank line needed after the rst directive. I'm not worried about missing something, the basics are covered nicely with docstrings and tests, as far as I can tell.
Through the magic of Edit: That was a lie; I forgot to |
Github won't let me reopen #2093.
+
Bugfix
because it now prints the .cfg path instead of ".sopel/foo"Description
This fixes two annoyances:
.cfg
when searching the config dir, but it doesn't when searching the current working directoryCombining these two, a user (me) will (does, regularly)
cd
to their non-default Sopel config dir, runsopel -c mybot
expecting it to load mybot.cfg and run, but instead have Sopel create ~/.sopel/ and ~/.sopel/mybot.cfg and start the config wizard.Searching
$PWD/{config}.{ext}
is straightforward, but not automatically running the wizard is probably subject to opinions. I changed it to the below output, but I would also be content with "Do you want to run the configuration wizard? y/n", or at least not creating the folder/file until the end of the wizard so the user has an opportunity to ^C without having to thenrm -rf ~/.sopel
Amended "couldn't find config":
Checklist
make qa
(runsmake quality
andmake test
)