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

Is get() not explicit enough? #258

Open
mozfreddyb opened this issue Feb 19, 2025 · 7 comments
Open

Is get() not explicit enough? #258

mozfreddyb opened this issue Feb 19, 2025 · 7 comments
Labels
discussion sanitizer-api issues with the API

Comments

@mozfreddyb
Copy link
Collaborator

During code review, our Firefox webidl reviewers pointed out that it's not entirely intuitive for a function get() to return a configuration dictionary.

One suggestion was to use a property instead of a function (e.g., readonly attribute SanitizerConfig configuration;), but I believe we had reasons why this ought to be a function?

@annevk
Copy link
Collaborator

annevk commented Feb 19, 2025

It should be a method as we want to return a new object each time. We could call it snapshot() I suppose.

(Currently toJSON() would also be an option, but I think that's too constraining going forward, when we might well have callbacks or some such.)

@mozfreddyb
Copy link
Collaborator Author

Yes, we agreed that JSON might be too constraining. A potential v2 feature was that other Sanitizers provide callbacks when sanitizing attributes/elements and that can not be represented in JSON.

@smaug----
Copy link

If it returns a "Configuration", calling it configuration() would be least surprising.

@annevk
Copy link
Collaborator

annevk commented Feb 25, 2025

I don't like that name. It returns the Sanitzer instance as a dictionary. We call that dictionary SanitizerConfig at the moment, but that's an internal name that doesn't really convey all that much information to web developers.

@lukewarlow
Copy link
Contributor

Perhaps leave it as get() and flag it as part of Tag Review?

@smaug----
Copy link

smaug---- commented Mar 5, 2025

Well, shouldn't API designers propose well named methods :) It shouldn't be up to the reviewers.
get() clearly doesn't indicate at all what it is returning.

Is it almost like toJSON() if it is really about Sanitizer as a dictionary?

@annevk
Copy link
Collaborator

annevk commented Mar 6, 2025

See #258 (comment).

@mozfreddyb mozfreddyb added sanitizer-api issues with the API discussion labels Mar 7, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
discussion sanitizer-api issues with the API
Projects
None yet
Development

No branches or pull requests

4 participants