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

config: don't assume ~/.sopel as dotdir #1404

Merged
merged 2 commits into from
Feb 7, 2019
Merged

config: don't assume ~/.sopel as dotdir #1404

merged 2 commits into from
Feb 7, 2019

Conversation

dgw
Copy link
Member

@dgw dgw commented Nov 9, 2018

Infer config directory from given config filename. I found this branch kicking around while looking for something else… how I forgot about it for six months, I have no idea.

Resolves #1243 (@alanhuang122 please confirm 😅)

@dgw dgw added the Tweak label Nov 9, 2018
@dgw dgw added this to the 7.0.0 milestone Nov 9, 2018
@alanhuang122
Copy link
Contributor

I think I was reporting for someone in the channel. I don't really remember the justification anymore. If I'm not misreading the code, this would work by specifying -c <file> (as opposed to being interactive), which doesn't sound 100% correct, but I wasn't the one having the issue :v

@dgw
Copy link
Member Author

dgw commented Dec 7, 2018

Yeah, you were. I'm glad I keep my IRC logs, because it means #1243 (comment) now has the whole conversation that led to you opening the issue in the first place.

I don't know if this is actually enough, based on that added context, but there's certainly time to add a new argument in Sopel 7 (--home or something) if not. Sopel's probably always going to try for ~/.sopel if run without arguments that change the config dir, though. Something has to be the default…

@Exirel
Copy link
Contributor

Exirel commented Dec 31, 2018

In #1429 I used a sopel.config.DEFAULT_HOMEDIR variable for the default home directory. I think it can be used here too.

@dgw
Copy link
Member Author

dgw commented Jan 31, 2019

@Exirel Want to drop a line note or two with suggested changes to show what you mean? Obviously I haven't touched this one in a while. 😅

@Exirel
Copy link
Contributor

Exirel commented Jan 31, 2019

Over here: https://github.com/sopel-irc/sopel/pull/1429/files#diff-7fa6048ad26c4e738f57cc6ed66b156aR37

From my point of view, one of the issue faced by Sopel's code is that default variable and business rules (or behavior rules/features) are all over the places. So I thought it could be a good idea to put this default variable (here, the default location to Sopel's homedir) where it belongs, ie. in sopel.config.DEFAULT_HOMEDIR.

We recently discussed on IRC about reorganizing the code without any change in feature, and I think this is one example of how I would change things: making sure each feature has one and only place to define their rules.

@Exirel
Copy link
Contributor

Exirel commented Jan 31, 2019

It also means that this:

dotdir = os.path.dirname(config) if config is not None else os.path.expanduser('~/.sopel')

Would become this:

dotdir = os.path.dirname(config) if config is not None else DEFAULT_HOMEDIR

@dgw
Copy link
Member Author

dgw commented Jan 31, 2019

That dotdir change is exactly what I thought it would be, thanks. :)

dgw added 2 commits February 5, 2019 10:14
Infer config directory from given config filename.

Resolves #1243
New global added in #1429 is a no-brainer to use in these spots.
@dgw
Copy link
Member Author

dgw commented Feb 5, 2019

Rebased and tweaked to use DEFAULT_HOMEDIR in the spots where it seemed appropriate.

@Exirel does this look better now?

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.

LGTM

@dgw dgw merged commit 4add965 into sopel-irc:master Feb 7, 2019
@dgw dgw deleted the 1243-hardcoded-dotdir branch February 7, 2019 06:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants