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

refactor: reformat and modernize large parts of the code base #111

Open
wants to merge 19 commits into
base: main
Choose a base branch
from

Conversation

adevade
Copy link
Contributor

@adevade adevade commented Jan 31, 2023

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 InvalidArgumentException everywhere, to get rid of the leading \
  • Reorder methods setUseHttps() and setSignKey() 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

@commit-lint
Copy link

commit-lint bot commented Jan 31, 2023

Code Refactoring

  • begin to start, and end to stop (0c83ec8)
  • order methods by constructor args (bd4091c)
  • import InvalidArgumentException class (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

  • update readme tests and readme content (a2136c6)
  • modernize and reformat test code (5b1995f)

Contributors

adevade

Commit-Lint commands

You can trigger Commit-Lint actions by commenting on this PR:

  • @Commit-Lint merge patch will merge dependabot PR on "patch" versions (X.X.Y - Y change)
  • @Commit-Lint merge minor will merge dependabot PR on "minor" versions (X.Y.Y - Y change)
  • @Commit-Lint merge major will merge dependabot PR on "major" versions (Y.Y.Y - Y change)
  • @Commit-Lint merge disable will desactivate merge dependabot PR
  • @Commit-Lint review will approve dependabot PR
  • @Commit-Lint stop review will stop approve dependabot PR

@adevade adevade marked this pull request as ready for review February 1, 2023 09:46
@adevade adevade requested a review from a team as a code owner February 1, 2023 09:46
@luqven
Copy link
Contributor

luqven commented Feb 1, 2023

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!

@adevade
Copy link
Contributor Author

adevade commented Feb 1, 2023

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants