-
Notifications
You must be signed in to change notification settings - Fork 205
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
Remove old constructor, add empty constructor #370
Conversation
Codecov Report
@@ Coverage Diff @@
## master #370 +/- ##
==========================================
- Coverage 94.51% 94.43% -0.09%
==========================================
Files 6 6
Lines 839 826 -13
Branches 85 79 -6
==========================================
- Hits 793 780 -13
Misses 34 34
Partials 12 12
Continue to review full report at Codecov.
|
@mganss The code coverage fails because it says it has less code coverage than before. On the diff on the codecov website it says the commit would remove code coverage for some lines but those are the lines that were removed. I don't know what's up with that. Maybe you could check it out? |
@vanillajonathan The coverage change is negligible (0.09%). Don't worry about it. |
Alright. Check the diff then, maybe @tiesont can do so too if he has any feedback. This will be a breaking change. +public HtmlSanitizer()
-public HtmlSanitizer(IEnumerable<string>? allowedTags = null, IEnumerable<string>? allowedSchemes = null,
- IEnumerable<string>? allowedAttributes = null, IEnumerable<string>? uriAttributes = null, IEnumerable<string>? allowedCssProperties = null) |
I think I'm mostly an interested bystander, but thank you for the mention. I do use HtmlSanitizer pretty heavily, in a few projects, to sanitize any content I've received from users, so I've been following your updates as much as I can. Without having used it yet, I really like the new options object and the (to me) simplified constructors. Basically, no complaints or nits from me. |
@vanillajonathan Sorry for the late response. LGTM. I'm wondering if all current use cases are supported in a non-surprising manner, especially the DI case discussed at #314 and #220, but AFAICT they are. |
Thanks! Yes, I believe the DI use cases are covered. Previously a workaround to deal with the DI injecting empty arrays were used, but now with the new DI-friendly parameterless constructor it always initializes the class with the default values just as before. Removing the old parameters means this change is a breaking change though since it breaks the API and existing users needs to rewrite the code to use the other constructor which takes in the options parameter. |
@vanillajonathan 🆗 Can this be merged now? |
@mganss Yes. |
This removes the old constructor in favor of the new constructor taking in an
HtmlSanitizerOptions
. It also adds a new empty constructor for convenience.