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

Remove old constructor, add empty constructor #370

Merged
merged 5 commits into from
Jul 21, 2022

Conversation

vanillajonathan
Copy link
Contributor

This removes the old constructor in favor of the new constructor taking in an HtmlSanitizerOptions. It also adds a new empty constructor for convenience.

@vanillajonathan vanillajonathan marked this pull request as draft July 15, 2022 21:06
@codecov
Copy link

codecov bot commented Jul 16, 2022

Codecov Report

Merging #370 (12b29d6) into master (07a8058) will decrease coverage by 0.08%.
The diff coverage is 100.00%.

@@            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              
Impacted Files Coverage Δ
src/HtmlSanitizer/HtmlSanitizer.cs 92.93% <100.00%> (-0.24%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 07a8058...12b29d6. Read the comment docs.

@vanillajonathan
Copy link
Contributor Author

@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?

@mganss
Copy link
Owner

mganss commented Jul 17, 2022

@vanillajonathan The coverage change is negligible (0.09%). Don't worry about it.

@vanillajonathan
Copy link
Contributor Author

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)

@tiesont
Copy link

tiesont commented Jul 17, 2022

Alright. Check the diff then, maybe @tiesont can do so too if he has any feedback.

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 vanillajonathan marked this pull request as ready for review July 18, 2022 14:37
@vanillajonathan
Copy link
Contributor Author

@tiesont Thank you for your feedback!

@mganss Check it out! What do you think?

@mganss
Copy link
Owner

mganss commented Jul 20, 2022

@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.

@vanillajonathan
Copy link
Contributor Author

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.

@mganss
Copy link
Owner

mganss commented Jul 21, 2022

@vanillajonathan 🆗 Can this be merged now?

@vanillajonathan
Copy link
Contributor Author

@mganss Yes.

@mganss mganss merged commit c283ac2 into mganss:master Jul 21, 2022
@vanillajonathan vanillajonathan deleted the add-preset branch July 21, 2022 12:18
@vanillajonathan vanillajonathan mentioned this pull request Aug 8, 2022
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.

3 participants