refactor: reformat and modernize large parts of the code base#111
Open
adevade wants to merge 19 commits intoimgix:mainfrom
Open
refactor: reformat and modernize large parts of the code base#111adevade wants to merge 19 commits intoimgix:mainfrom
adevade wants to merge 19 commits intoimgix:mainfrom
Conversation
Code Refactoring
Styles
TestsContributorsCommit-Lint commandsYou can trigger Commit-Lint actions by commenting on this PR:
|
Contributor
|
Hey, @adevade lots of interesting changes here 👍🏼 . Will take us a hot minute to review. I wanted to double-check and see if it made sense to review this per-commit or if just reading the diff is better. I want to tackle this piecemeal if possible. IE, does each commit stand on its own or do they depend on each other for passing tests? It's fine either way, I just want to avoid reading over work that gets re-written a commit or two later. Thanks for your hard work on this, excited for the chance to review it! |
4 tasks
Contributor
Author
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
This is a rather large PR, with some tweaks to both the tests and source code. However, all tests are still passing with 100% coverage. There should be no breaking changes.
General
InvalidArgumentExceptioneverywhere, to get rid of the leading\setUseHttps()andsetSignKey()to match constructor orderUrlBuilder::VERSIONconstantI created a public constant on the main class, so it can be used for both the library param and for all tests. This removes the
$versionproperty, but since that was private it was only used withing the class.Constructor Promotion
In the constructor of both
UrlBuilderandUrlHelperI've refactored to using Constructor Promotion. In simple words, it lets you declare the property directly in the constructor.Null Coalescing Operator
This is a shorter form for checking if a variable -- or a key in an array -- is set, and setting a fallback value if it's not. (Docs)
Building srcsets
Earlier the srcsets were built directly using string concatenation. I've switched to building up an array of the individual strings and then gluing them together in the return.
I've switched from
forloops toforeachloops, since they're generally easier to read (IMO).Also, in
createDPRSrcSet, I'm using the keys fromDPR_QUALITIESinstead of the separateTARGET_RATIOSwhich seems superfluous.README
I've updated the README to use the same formatting as the rest of the code. Also some of the examples were out of date with the actual results. I don't know if the test file for the README is needed, really? Easy to forget updating it when making changes.
Tests
I've defined a
HOSTconstant to ensure the tests call the same domain. Before it was bothdemo.imgix.netanddemos.imgix.net.Most tests now use named arguments to keep the default values for
useHttpsandsignKey, and disabling the library param. Ex.new UrlBuilder('demos.imgix.net', includeLibraryParam: false)$paramsand$optionsarrays has been inlined.The library param has been disabled on many tests, where it wasn't needed. The reduces noise and makes it easier to se what is really being tested.
Some methods has been reordered in
UrlBuilderTest.Checklist