refactor: reformat and modernize large parts of the code base
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
- More code style formatting
- Rename and remove unnecessary variables
- Import
InvalidArgumentExceptioneverywhere, to get rid of the leading\ - Reorder methods
setUseHttps()andsetSignKey()to match constructor order
UrlBuilder::VERSION constant
I 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 $version property, but since that was private it was only used withing the class.
Constructor Promotion
In the constructor of both UrlBuilder and UrlHelper I'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)
// Before
$widths = isset($options['widths']) ? $options['widths'] : null;
// After
$widths = $options['widths'] ?? null;
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 for loops to foreach loops, since they're generally easier to read (IMO).
Also, in createDPRSrcSet, I'm using the keys from DPR_QUALITIES instead of the separate TARGET_RATIOS which 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 HOST constant to ensure the tests call the same domain. Before it was both demo.imgix.net and demos.imgix.net.
Most tests now use named arguments to keep the default values for useHttps and signKey, and disabling the library param. Ex. new UrlBuilder('demos.imgix.net', includeLibraryParam: false)
$params and $options arrays 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
- [x] Read the contributing guidelines.
- [x] Each commit follows the Conventional Commit spec format.
- [x] Update the readme.
- [x] All existing unit tests are still passing.
Code Refactoring
begintostart, andendtostop(0c83ec8)- order methods by constructor args (bd4091c)
- import
InvalidArgumentExceptionclass (b9ff408) - single quote strings, short syntax arrays (d53d680)
- define and use public version constant on UrlBuilder (e4cc245)
- use constructor property promotion (a0e1ffc)
- remove unused variable (f74ec9f)
- modernize code in createSrcSet (32ccc4e)
- replace for with foreach and arrays (998f0cd)
- inline variable (58af04c)
- use null coalescing operator (f08424f)
- update variable name (2fb9493)
- string interpolation over concatenation (d588980)
Styles
- format and remove unused comments (920aa71)
- update comment (8c724a4)
- remove comment and format array (0c83a98)
- fix comma spacing (c4ef328)
Tests
Contributors
Commit-Lint commands
You can trigger Commit-Lint actions by commenting on this PR:
@Commit-Lint merge patchwill merge dependabot PR on "patch" versions (X.X.Y - Y change)@Commit-Lint merge minorwill merge dependabot PR on "minor" versions (X.Y.Y - Y change)@Commit-Lint merge majorwill merge dependabot PR on "major" versions (Y.Y.Y - Y change)@Commit-Lint merge disablewill desactivate merge dependabot PR@Commit-Lint reviewwill approve dependabot PR@Commit-Lint stop reviewwill stop approve dependabot PR
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!
@luqven Each commit should work by itself, with passing tests, I believe. Just some small bits being overwritten by later commits. 👍
EDIT: The second to last commit (updating the tests) is the largest by far: 5b1995f . The rest is small bits and pieces.