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

Minor improvements #90

Merged
merged 1 commit into from
Dec 15, 2021
Merged

Minor improvements #90

merged 1 commit into from
Dec 15, 2021

Conversation

Bond-009
Copy link
Member

No description provided.

@h1dden-da3m0n h1dden-da3m0n added chore This PR contains repository maintenance changes tests This PR adds or imposes tests (e.g. coverage, quality, ...) labels Dec 14, 2021
Copy link
Contributor

@h1dden-da3m0n h1dden-da3m0n left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I assume using a regular Directory and a custom query builder is yields more performance?
And maybe a nit, but a negative test for that func would be cool, but I must not complain, I write way too few tests :S

@h1dden-da3m0n
Copy link
Contributor

h1dden-da3m0n commented Dec 15, 2021

@MBR-0001 you have been the quite active in this plugin, mind giving this another look?

@Bond-009
Copy link
Member Author

The custom query builder isn't for performance (although it may be faster) it's just less hacky then the current code.
System.Web.HttpUtility.ParseQueryString(string.Empty) was used to get an instance of HttpQSCollection which is an internal class of .net, it also escapes spaces with %20, which is why it needed another hack on top .Replace("%20", "+", StringComparison.Ordinal)

@h1dden-da3m0n
Copy link
Contributor

Thank you for clarifying, and I have to admit that I did not think of that nor would have been able to notice 😅

@MBR-0001
Copy link
Contributor

@MBR-0001 you have been the quite active in this plugin, mind giving this another look?

I ran a few tests and everything worked as expected, I didn't look at the moviehash code but it appears to be creating same hashes as before... The query string code follows all 5 (or 4? idk) rules and looks much cleaner (glad the %20 hack is gone lol)

so, in conclusion, looks good 👍

@h1dden-da3m0n h1dden-da3m0n merged commit a47fec9 into master Dec 15, 2021
@h1dden-da3m0n h1dden-da3m0n deleted the minor branch December 15, 2021 22:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
chore This PR contains repository maintenance changes tests This PR adds or imposes tests (e.g. coverage, quality, ...)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants