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

[11.x] use promoted properties #53807

Merged
merged 3 commits into from
Dec 10, 2024

Conversation

browner12
Copy link
Contributor

this commit uses promoted properties to remove a lot of boilerplate code.

I avoided promoting any public properties that lacked type-hints, but had type-hints in the constructor signature. while unlikely to cause issues, this technically is a change in behavior, and should be handled against the master branch.

I did upgrade protected properties that lacked type hints to the type hint provided in the constructor signature. This should be okay because even though the properties themselves weren't type-hinted, the only way to update them was through a type-hinted constructor.

some styling decisions made in this PR:

  • ALWAYS use multiline constructors, with each parameter on a new line. provides for very simple to grok git diffs.
  • use a trailing comma in multiline constructors
  • leave in constructor docblocks for now. could/should be removed eventually

there are many more to go, but wanted to test the waters with how this would be received first.

this commit uses promoted properties to remove a lot of boilerplate code.

I avoided promoting any `public` properties that lacked type-hints, but had type-hints in the constructor signature. while unlikely to cause issues, this technically is a change in behavior, and should be handled against the `master` branch.

I **did** upgrade `protected` properties that lacked type hints to the type hint provided in the constructor signature. This should be okay because even though the properties themselves weren't type-hinted, the only way to update them was through a type-hinted constructor.

some styling decisions made in this PR:

- ALWAYS use multiline constructors, with each parameter on a new line
- use a trailing comma in multiline constructors
- leave in constructor docblocks for now. could/should be removed eventually
@browner12
Copy link
Contributor Author

random question, but is there a reason we're still using StyleCI?

it would be great to have a configuration option to allow for single line empty bodies (as you often see in constructors with promoted properties), but as far as I can tell, that isn't possible.

would it make sense to use Pint to format the framework?

@taylorotwell taylorotwell merged commit 69b19ce into laravel:11.x Dec 10, 2024
38 checks passed
taylorotwell added a commit that referenced this pull request Dec 10, 2024
taylorotwell added a commit that referenced this pull request Dec 10, 2024
@taylorotwell
Copy link
Member

@browner12 can you actually send this to master?

@crynobone
Copy link
Member

would it make sense to use Pint to format the framework?

StyleCI rarely changes the laravel preset making reformatting from a PR a small change, Pint however still accepts new default for laravel preset making it hard to maintain (merge conflict, out of sync PRs etc)

@browner12 browner12 deleted the AB-modern-constructors branch December 10, 2024 16:05
@cosmastech
Copy link
Contributor

StyleCI rarely changes the laravel preset making reformatting from a PR a small change, Pint however still accepts new default for laravel preset making it hard to maintain (merge conflict, out of sync PRs etc)

Glad to know that folks who work at Laravel see this as a problem too. @crynobone

Using Pint with the laravel default is a non-starter for me, since it's liable to change on a release of pint, increasing PR review burden and making change history a nightmare.

The workaround I've taken is to just copy all of the defaults that are currently set and create our own pint.json file. This could be an option for the framework as well.

@timacdonald
Copy link
Member

@cosmastech, I recommend pinning a specific version of Pint, rather than a range. That way you choose when to update and receive potential rule changes.

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.

6 participants