Skip to content

config, doc, test: clarify nick@host format of owner, admins settings#2733

Open
SnoopJ wants to merge 2 commits intosopel-irc:masterfrom
SnoopJ:chore/test-and-doc-owner-host
Open

config, doc, test: clarify nick@host format of owner, admins settings#2733
SnoopJ wants to merge 2 commits intosopel-irc:masterfrom
SnoopJ:chore/test-and-doc-owner-host

Conversation

@SnoopJ
Copy link
Copy Markdown
Contributor

@SnoopJ SnoopJ commented Apr 8, 2026

Description

This changeset clarifies in the documentation that the core config field owner may be given as nick@host as well as a nick, and also covers this previously-untested usage in the Trigger tests. (Edit: this applies to the admins field as well)

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 lint and make test)
    • 1451 passed, 8 xfailed, 1 warning in 47.36s
  • I have tested the functionality of the things this change touches

Remarks for reviewer

In moving the dummy configs to a fixture, I originally wanted to move the template out of the module namespace and into the fixture, relying on textwrap.dedent() to take care of the indentation:

@pytest.fixture
def tmpconfig(configfactory) -> Config:
    return textwrap.dedent(f"""
        ...
    """

But this approach can cause trouble with trying to add a field later if also using an f-string, so I settled on a fixture that returns source. In particular, the thing I stumbled on was wanting to write:

def test_ircv3_account_tag_trigger(nick, tmpconfig_src, configfactory):
    line = '@account=bar :Nick_Is_Not_Foo!foo@example.com PRIVMSG #Sopel :Hello, world'
    pretrigger = PreTrigger(nick, line)

    # NOTE: only first line of tmpconfig_src has the expected indent level
    # so the result ends up exactly the same as without the dedent()
    tmp_config_account = dedent(f"""
        {tmpconfig_src}
        owner_account = bar
    """)

I don't feel strongly about eliminating the module-level TMP_CONFIG and if desired I can update this PR to preserve it.

@dgw dgw changed the title Chore/test and doc owner host config, doc, test: clarify nick@host format of owner settings Apr 10, 2026
@dgw dgw changed the title config, doc, test: clarify nick@host format of owner settings config, doc, test: clarify nick@host format of owner setting Apr 10, 2026
Copy link
Copy Markdown
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.

In addition to the style tweaks mentioned as line notes, let's either include core.admins in this as well (it also uses match_host_or_nick()) or immediately follow up with another PR to do so.

Comment thread sopel/config/core_section.py Outdated
Comment thread test/test_trigger.py Outdated
@SnoopJ SnoopJ changed the title config, doc, test: clarify nick@host format of owner setting config, doc, test: clarify nick@host format of owner, admins settings Apr 12, 2026
@SnoopJ SnoopJ force-pushed the chore/test-and-doc-owner-host branch from 9a820e5 to 2b07758 Compare April 12, 2026 17:51
Copy link
Copy Markdown
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.

Discretizing tests for the different cases will probably happen in whatever patch addresses #2734 (thanks for opening that). For now this is a good step forward.

10 commits is a lot, with the fixups and stuff, so this is your 👍 from me to squash things. 😸

SnoopJ and others added 2 commits April 12, 2026 19:20
Co-authored-by: dgw <dgw@technobabbl.es>
@SnoopJ SnoopJ force-pushed the chore/test-and-doc-owner-host branch from 2b07758 to c4fe40c Compare April 12, 2026 23:21
@SnoopJ
Copy link
Copy Markdown
Contributor Author

SnoopJ commented Apr 12, 2026

I have a smaller patch for #2734 that "just" adds a new test, will open that in a moment, but I don't think it's any big problem if we change the doc in one changeset and address the problem of test coverage in another, since that gap has been there a while anyway.

@dgw
Copy link
Copy Markdown
Member

dgw commented Apr 14, 2026

GH doesn't support relationships between PRs and issues (why?) but consider this blocked by #2736 for now. (@Exirel you're still welcome to review; certainly the tests are useful unless we get rid of the behavior entirely.)

@dgw
Copy link
Copy Markdown
Member

dgw commented Apr 14, 2026

After what one might call much discussion on IRC, we seem to have an alternative plan involving a new config value and deprecating certain value patterns of the existing owner setting. I will describe it in #2736, perhaps mirrored in #2587 (or just linked from there) as it's all related.

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