Skip to content

cli: fix SOPEL_CONFIG env var broken usage #1882

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.

Sign up for GitHub

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

Jump to bottom
Merged
dgw merged 2 commits into from
Jun 8, 2020
Merged

cli: fix SOPEL_CONFIG env var broken usage #1882

dgw merged 2 commits into from
Jun 8, 2020

Conversation

@Exirel
Copy link
Contributor

@Exirel Exirel commented Jun 2, 2020 •
edited
Loading

Description

The code around config loading for command lines didn't work properly: it ignored the SOPEL_CONFIG env var. So I fixed it, and added some tests too.

Its usage is now documented in the help message for the -c/--config option.

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

@Exirel Exirel added the Bugfix Generally, PRs that reference (and fix) one or more issue(s) label Jun 2, 2020
@Exirel Exirel added this to the 7.1.0 milestone Jun 2, 2020
@Exirel Exirel requested a review from a team June 2, 2020 15:05
@Exirel
Copy link
Contributor Author

Exirel commented Jun 2, 2020

Travis doesn't report it, but the tests pass. https://travis-ci.org/github/sopel-irc/sopel/builds/693891907

dgw
dgw requested changes Jun 2, 2020
View reviewed changes
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.

Travis failing to send updates about build status back to GitHub happens sometimes. Wait a bit, fix these typos, and update the branch to make it try again. 😝

Could also be a good time to split testing the color stuff in cli.utils into a separate commit (not PR) if you're up for it. Lumping that in feels a bit odd, though everything else seems pretty interrelated.

@Exirel
Copy link
Contributor Author

Exirel commented Jun 2, 2020

Could also be a good time to split testing the color stuff in cli.utils into a separate commit

Oh right, I started this PR as "let's add some tests", and then I found this bug and fixed it. So it's an artefact of that process. Will see what I can do with that.

@Exirel Exirel requested a review from dgw June 2, 2020 15:29
@Exirel Exirel force-pushed the cli-config-env-var branch from 419a54b to d3b9a93 Compare June 2, 2020 16:02
@Exirel
Copy link
Contributor Author

Exirel commented Jun 2, 2020

I split the commits as expected! Let's hope Travis will dutifully report its results now.

dgw
dgw approved these changes Jun 3, 2020
View reviewed changes
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.

Looks good, but Travis never reported status even though the whole build passed. :|

@dgw
Copy link
Member

dgw commented Jun 8, 2020

Don't remember if this was the first or second time Travis broke recently, but it's fixed now. Merge time.

@dgw dgw merged commit b575787 into sopel-irc:master Jun 8, 2020
@Exirel Exirel deleted the cli-config-env-var branch June 23, 2020 12:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@dgw dgw dgw approved these changes

Assignees

No one assigned

Labels

Bugfix Generally, PRs that reference (and fix) one or more issue(s) High Priority

Projects

None yet

Milestone

7.1.0

Development

Successfully merging this pull request may close these issues.

2 participants

@Exirel @dgw