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

cli: look harder for $PWD/cfg, don't automatically start wizard #2299

Merged
merged 1 commit into from
Mar 1, 2023

Conversation

half-duplex
Copy link
Member

Github won't let me reopen #2093.
+Bugfix because it now prints the .cfg path instead of ".sopel/foo"

Description

This fixes two annoyances:

  • Sopel automatically adds .cfg when searching the config dir, but it doesn't when searching the current working directory
  • If the config file is not found, it currently automatically starts the configuration wizard.

Combining these two, a user (me) will (does, regularly) cd to their non-default Sopel config dir, run sopel -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 then rm -rf ~/.sopel

Amended "couldn't find config":

$ sopel
ConfigurationError: Unable to find the configuration file /home/user/.sopel/default.cfg
If you're just setting up Sopel, welcome! You can use `sopel configure` to get started easily.

Checklist

  • I have read CONTRIBUTING.md
  • I can and do license this contribution under the EFLv2
  • No issues are reported by make qa (runs make quality and make test)
  • I have tested the functionality of the things this change touches

@half-duplex half-duplex added Tweak Bugfix Generally, PRs that reference (and fix) one or more issue(s) labels Jun 8, 2022
@half-duplex half-duplex requested a review from Exirel June 8, 2022 18:53
Copy link
Contributor

@Exirel Exirel left a 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.

sopel/cli/run.py Outdated Show resolved Hide resolved
sopel/cli/utils.py Show resolved Hide resolved
sopel/cli/run.py Show resolved Hide resolved
Copy link
Contributor

@Exirel Exirel left a 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.

sopel/cli/utils.py Outdated Show resolved Hide resolved
@Exirel Exirel added the Breaking Change Stuff that probably should be mentioned in a migration guide label Oct 27, 2022
@Exirel
Copy link
Contributor

Exirel commented Oct 27, 2022

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.

@half-duplex half-duplex requested a review from Exirel February 28, 2023 17:27
Copy link
Contributor

@Exirel Exirel left a 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.

@dgw
Copy link
Member

dgw commented Mar 1, 2023

Through the magic of git merge --no-ff --no-commit, I will take care of the whitespace without needing another force-push.

Edit: That was a lie; I forgot to git add the whitespace change before committing and pushing the merge. GJ me. 😅

@dgw dgw merged commit 292d222 into sopel-irc:master Mar 1, 2023
dgw added a commit that referenced this pull request Mar 2, 2023
@dgw dgw mentioned this pull request Mar 2, 2023
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Breaking Change Stuff that probably should be mentioned in a migration guide Bugfix Generally, PRs that reference (and fix) one or more issue(s) Tweak
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants