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

Use shorthand nullability typings for better readability #5733

Open
wants to merge 5 commits into
base: next
Choose a base branch
from

Conversation

jnoordsij
Copy link
Contributor

WHY

BEFORE - What was wrong? What was happening before this PR?

When working on #5732, I encountered some other (nullable) types that could be simplified, to improve readability.

AFTER - What is happening after this PR?

Cleaner code.

HOW

How did you achieve that, in technical terms?

Use shorthand.

Is it a breaking change?

No (or not really); only the change in RegisterUploadEvents::handle is a change in behavior, due to no longer allowing null; however explicit null passing seems unlikely and unwanted here.

How can we test the before & after?

??

If the PR has changes in multiple repos please provide the command to checkout all branches, eg.:

git checkout "dev-branch-name" &&
cd vendor/backpack/crud && git checkout crud-branch-name &&
cd ../pro && git checkout pro-branch-name &&
cd ../../..

pxpm and others added 4 commits December 3, 2024 10:19
* add method to get ajax uploaders

* Apply fixes from StyleCI

[ci skip] [skip ci]

* use an abstract class

* wip

* Apply fixes from StyleCI

[ci skip] [skip ci]

* refactor uploaders

* Apply fixes from StyleCI

[ci skip] [skip ci]

* refactor rules

* Apply fixes from StyleCI

[ci skip] [skip ci]

* move ajax to PRO, cleanup

* Apply fixes from StyleCI

[ci skip] [skip ci]

* make attributes available for all subfields

* fix tests

* wip

* Apply fixes from StyleCI

[ci skip] [skip ci]

* upload multiple and upload properly working 🙏

* fixes

* Apply fixes from StyleCI

[ci skip] [skip ci]

* allow the configuration of valueWithoutPath call.

* fix valid upload inside repeatables

* Apply fixes from StyleCI

[ci skip] [skip ci]

* fix condition

* cleanup

* fix

* Apply fixes from StyleCI

[ci skip] [skip ci]

* fix use case for enabling validation after entry is created

* Apply fixes from StyleCI

[ci skip] [skip ci]

* dont save array keys

* fix ajax validation

* fix validation messages

* Apply fixes from StyleCI

[ci skip] [skip ci]

* fixes ValidUpload

* Apply fixes from StyleCI

[ci skip] [skip ci]

* dont json encode if casted in the model

* Apply fixes from StyleCI

[ci skip] [skip ci]

* fix previous file identification in repeatable

* Apply fixes from StyleCI

[ci skip] [skip ci]

* fix getting values

* Apply fixes from StyleCI

[ci skip] [skip ci]

* add fake fields support

* Apply fixes from StyleCI

[ci skip] [skip ci]

* wip add uploaders tests

* Apply fixes from StyleCI

[ci skip] [skip ci]

* wip

* Apply fixes from StyleCI

[ci skip] [skip ci]

* add pro columns

* fix test suite

* fix tests

* ffix tests

* remove unused test views

* add uploaders to test coverage

* Apply fixes from StyleCI

[ci skip] [skip ci]

* add coverage folder to gitignore

* make tests run faster by not reloading db when not necessary

* add coverage to validation tests

* add fake tests to uploaders

* Apply fixes from StyleCI

[ci skip] [skip ci]

* add more tests

* Apply fixes from StyleCI

[ci skip] [skip ci]

* wip

* Apply fixes from StyleCI

[ci skip] [skip ci]

* wip

* wip

* Apply fixes from StyleCI

[ci skip] [skip ci]

* wip

* Apply fixes from StyleCI

[ci skip] [skip ci]

* add more upload assets

* fixes

* Apply fixes from StyleCI

[ci skip] [skip ci]

* fix single file

* Apply fixes from StyleCI

[ci skip] [skip ci]

* add image column

* fix tests

* Apply fixes from StyleCI

[ci skip] [skip ci]

* remove hardcoded macro names

* Apply fixes from StyleCI

[ci skip] [skip ci]

* remove double loop, fix single file uploader

* Apply fixes from StyleCI

[ci skip] [skip ci]

* use a big increments and unsigned for primary key

* handle pivot file deletion

* Apply fixes from StyleCI

[ci skip] [skip ci]

* register events for relation models

* Apply fixes from StyleCI

[ci skip] [skip ci]

* fix typo

* Apply fixes from StyleCI

[ci skip] [skip ci]

* fix relationship uploaders

* Apply fixes from StyleCI

[ci skip] [skip ci]

* wip

* Apply fixes from StyleCI

[ci skip] [skip ci]

* wip

* Apply fixes from StyleCI

[ci skip] [skip ci]

* update temporary time key

* save objects in the macro

---------

Co-authored-by: StyleCI Bot <[email protected]>
* wip

* clean up

* add get uploadedFilesFromRequest to clean up uploaders

* Apply fixes from StyleCI

[ci skip] [skip ci]

* fix type cast

* add summernote uploader

* update summernote

* add summernote

* add more test assets

---------

Co-authored-by: StyleCI Bot <[email protected]>
@pxpm
Copy link
Contributor

pxpm commented Dec 18, 2024

Hey @jnoordsij thanks for the contribution. 🙏

Would it be too much to ask to merge both PR's into a single one, since they address the same issue ?

If you don't have time/will to do it, let me know and I can do that, I'd rather merge this as a single PR 👍

Cheers

@jnoordsij
Copy link
Contributor Author

jnoordsij commented Dec 18, 2024

I'm all fine with adding the commit here to #5732. I've deliberately split them, as #5732 now only contains essential changes that are practically identical to the previous state, just solving the deprecations.

This MR goes slightly further by actually introducing a single behavior change, and thus should arguably be handled with a little more care. Moreover, the changes are merely cosmetic and are not required to actually fix deprecation messages, like #5732.

However, if you prefer handling it all together at once, just let me know and I'll cherry-pick this onto #5732.

@pxpm
Copy link
Contributor

pxpm commented Dec 19, 2024

Thanks for the clarification. 🙏

I agree then, let's merge the other PR in v6, and mark this for v7, I agree that it's unlike that anyone is using null in that parameter, but it was overlooked and to be safe we should do it in v7. We already have some BC's in internal code for uploaders in v7.

Thanks @jnoordsij 🙏

@pxpm pxpm added the v7 label Dec 19, 2024
@pxpm pxpm changed the base branch from main to next December 19, 2024 12:30
@jnoordsij jnoordsij force-pushed the null-typing-improvements branch from af9feb4 to fff82d0 Compare December 19, 2024 14:52
@jnoordsij
Copy link
Contributor Author

Sounds great! I've rebased this to resolve conflicts with the new target branch.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: No status
Development

Successfully merging this pull request may close these issues.

4 participants