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: add --config-dir common option #1598

Merged
merged 4 commits into from
Sep 4, 2019
Merged

Conversation

Exirel
Copy link
Contributor

@Exirel Exirel commented May 12, 2019

I used to left a comment like this:

# TODO: allow to use a different homedir

But no more. Now, you can use --config-dir <configdir> for that. This option provides the location to look for configuration file. Note that this does not replace and must not be confused with the settings's homedir, which is used internally by Sopel once it is started.

Edit: used to be -H/--homedir <homedir>

@dgw
Copy link
Member

dgw commented May 12, 2019

the settings's

Ahem, config's* (now I see a bit more inside that brain of yours, replacing config with settings all over the place even though it just makes things more confusing sometimes)

Note that this does not replace and must not be confused with

This is a design consideration. Should we not call this a "homedir"? Should this "homedir" override the other one if given? If this is only for finding config files, maybe it should be called --config-dir instead…

Just thinking aloud, here. I'm very much in favor of having this option; it's just, as usual, the details. 😅

@Exirel
Copy link
Contributor Author

Exirel commented May 12, 2019

This is a design consideration. Should we not call this a "homedir"? Should this "homedir" override the other one if given? If this is only for finding config files, maybe it should be called --config-dir instead…

Hm. Yeah, probably. It was called "homedir" long before I added the constant DEFAULT_HOMEDIR (hence my choice of name), so I kept that. But you're probably right. To be honest, how configuration are found is bonkers, because it's probably the first time I ever see a software asking for the name of the configuration, instead of the path.

That beind said, yes, let's go for  --config-dir, that sound reasonable—and way better.

@dgw
Copy link
Member

dgw commented May 12, 2019

I mean, we can totally throw the whole "config name" thing in the trash along with the CLI redesign. Ordinarily I'd say that should wait for Sopel 8 and not add any more to 7's plate, but the time to do it would be in 7 with the CLI changes already being made…

Is it useful enough to have a "list configs" function, that we want to keep the way things work now? That is, specifying a config directory and name separately? Or should we nix that and make the new CLI do -c = --config = /path/to/config/file.cfg (or .yml… 😏)?

@Exirel
Copy link
Contributor Author

Exirel commented May 12, 2019

Hmmmm yeah. I think I'm on board with that idea.

However, this is heading into the "hijack a PR to talk about an issue" territory, and it's a dangerous place.

@dgw
Copy link
Member

dgw commented May 12, 2019

However, this is heading into the "hijack a PR to talk about an issue" territory, and it's a dangerous place.

I guess I should at least try to avoid hypocrisy, huh? Though it's relevant because if we choose to go that alternate route, this PR isn't needed and should be closed (or overwritten with that solution). 😉

@Exirel
Copy link
Contributor Author

Exirel commented May 12, 2019

if we choose to go that alternate route, this PR isn't needed and should be closed

Indeed. At the moment, to be honest, I don't have any upgrade path in mind. I wish I already knew how to migrate from -c name but I don't. Even if I don't like this, even if I think it's bonkers, I prefer to take a safer, longer path for the user experience's sake: I don't see how not to break the system with such a change.

Heh, I guess sometimes I can't have it all.

@Exirel Exirel force-pushed the cli-homedir-option branch from d3d12ea to 3ec64c3 Compare May 12, 2019 13:16
@Exirel Exirel changed the title cli: add -H/--homedir common option cli: add --config-dir common option May 12, 2019
@Exirel
Copy link
Contributor Author

Exirel commented May 12, 2019

@dgw changed to --config-dir

@Exirel
Copy link
Contributor Author

Exirel commented May 15, 2019

After a second thought: support for -c name is unrelated to "where are the config file?" For example, we could have -c name.cfg and still have --config-dir /etc/sopel. From my point of view, I could have a set of configurations in a shared folder, and run multiple instance of Sopel using this kind of command line:

export SOPEL_CONFIG_DIR = "/etc/sopel"
sopel -c freenode.cfg --config-dir $SOPEL_CONFIG_DIR
sopel -c quakenet.cfg --config-dir $SOPEL_CONFIG_DIR
sopel -c private.cfg --config-dir $SOPEL_CONFIG_DIR

This is exactly the kind of thing I do for other services. So, heh. You can review that, it's not a feature that will disappear when we rework the -c name option. ;-)

Copy link
Member

@dgw dgw left a comment

Choose a reason for hiding this comment

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

There's nothing huge here. I just made some mostly-banal English tweaks; the PR itself is nice.

I need to remember to update the migration guide with the deprecation of --list though.

sopel/cli/config.py Outdated Show resolved Hide resolved
sopel/cli/config.py Outdated Show resolved Hide resolved
sopel/cli/run.py Outdated Show resolved Hide resolved
sopel/cli/utils.py Outdated Show resolved Hide resolved
sopel/cli/utils.py Outdated Show resolved Hide resolved
sopel/cli/utils.py Outdated Show resolved Hide resolved
sopel/cli/config.py Outdated Show resolved Hide resolved
@Exirel
Copy link
Contributor Author

Exirel commented Jul 5, 2019

@dgw All good! Also:

Someone remind me to add this in the v7 migration guide's CLI overhaul section.

@Exirel Exirel requested a review from dgw July 9, 2019 21:30
@Exirel Exirel force-pushed the cli-homedir-option branch from c192d91 to e8808e2 Compare July 17, 2019 18:03
Copy link
Member

@dgw dgw left a comment

Choose a reason for hiding this comment

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

Pretty minor stuff, all things considered. This will be a good addition to the CLI!

sopel/cli/config.py Outdated Show resolved Hide resolved
sopel/cli/config.py Outdated Show resolved Hide resolved
sopel/cli/config.py Show resolved Hide resolved
sopel/cli/run.py Outdated Show resolved Hide resolved
sopel/cli/run.py Outdated Show resolved Hide resolved
sopel/cli/utils.py Outdated Show resolved Hide resolved
test/cli/test_cli_utils.py Outdated Show resolved Hide resolved
@Exirel Exirel force-pushed the cli-homedir-option branch from e8808e2 to f233c78 Compare August 31, 2019 14:21
@Exirel
Copy link
Contributor Author

Exirel commented Aug 31, 2019

@dgw Aside from the random error (caused by a test that use an online resource), it's all good to a new review.

@Exirel Exirel requested a review from dgw August 31, 2019 14:26
Copy link
Member

@dgw dgw left a comment

Choose a reason for hiding this comment

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

Tiny review is tiny. 😹

sopel/cli/run.py Outdated Show resolved Hide resolved
test/cli/test_cli_utils.py Outdated Show resolved Hide resolved
@Exirel Exirel force-pushed the cli-homedir-option branch from f28c906 to ac56884 Compare September 2, 2019 19:23
@Exirel
Copy link
Contributor Author

Exirel commented Sep 2, 2019

@dgw I fixed the double-backtick problem. All it needs now is #1686 to be merged so tests pass with Python 2.

Copy link
Member

@dgw dgw left a comment

Choose a reason for hiding this comment

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

And the tests pass now, with #1686 merged. Thanks for this!

@dgw dgw merged commit bff1832 into sopel-irc:master Sep 4, 2019
@Exirel Exirel deleted the cli-homedir-option branch September 5, 2019 09:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants