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

Consider creating an "effective sanitizer configuration" during construction #153

Closed
mozfreddyb opened this issue Jun 10, 2022 · 2 comments
Milestone

Comments

@mozfreddyb
Copy link
Collaborator

I noticed that we do a lot of if/else dance during sanitization that make the algorithms a bit less clear and are sometimes brittle.

I suggest we separate the concern of "taking a config" form the concern of sanitization by building an config dictionary based on the supplied arguments (and filling the missing properties with defaults).

Then we only need to look into that one specific config during sanitization, instead of doing the "is it present, if not look elsewhere" dance.

I believe we discussed this before, but it occurred to me more closely when re-reviewing the spec.

@otherdaniel
Copy link
Collaborator

For a while, the spec was written that way, and for the reasons you give. I dropped it, since it didn't seem to simplify anything after all: I needed all the same conditions and checks, I just had them in a different place. That way, the "effective configuration" was merely an indirection rather than a valid abstraction. So all it accomplished was that when figuring out what to do, I had to additionally look in another place.

Would be happy to try again, if we have a good take on how to define an effective configuration.

@mozfreddyb mozfreddyb self-assigned this Jun 15, 2022
@annevk annevk added the v1 label Oct 18, 2023
@mozfreddyb mozfreddyb added this to the v1 milestone Jan 23, 2024
@mozfreddyb mozfreddyb removed their assignment Feb 21, 2024
@mozfreddyb
Copy link
Collaborator Author

This should be resolved due to the config canonicalization coming up in #208. This is mostly resolved (and there is agreement).

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

No branches or pull requests

3 participants