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

Tweaks to swap style builder and related types #28

Merged
merged 4 commits into from
May 1, 2024
Merged

Tweaks to swap style builder and related types #28

merged 4 commits into from
May 1, 2024

Conversation

egil
Copy link
Owner

@egil egil commented May 1, 2024

These were the changes I was thinking about for your PR. See any issues with them @tanczosm?

@tanczosm
Copy link
Collaborator

tanczosm commented May 1, 2024

HtmxResponse line 145:

    public HtmxResponse Reswap(string modifier)
    {
        if (string.IsNullOrWhiteSpace(modifier))
        {
            headers.Remove(HtmxResponseHeaderNames.Reswap);
        }
        else
        {
            headers[HtmxResponseHeaderNames.Reswap] = modifier;
        }

        return this;
    }

The remove aspect for headers is an undocumented side effect which I'm fine with adding but just augment the xmldoc. Also, on line 171 you call Reswap(modifier) in which modifier is nullable even though the parameter of that Reswap overload isn't nullable. Perhaps use either Reswap(modifier ?? string.Empty) or throw an exception. Being honest, I'm thinking if you call reswap with a SwapStyle.Default and no modifier then it's purely a mistake that would have no effect and might be worth alerting the developer via the thrown exception.

Also is AjaxContext impacted by this at all with the Swap property?

@egil
Copy link
Owner Author

egil commented May 1, 2024

Good point. Ill throw an argument null exception if modifier is null or whitespace.

As for AjaxContent, lets cover that in another PR.

@egil egil merged commit 74747ed into main May 1, 2024
6 checks passed
@egil egil deleted the tweaks branch May 1, 2024 23:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants