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

V4 is getting ready #866

Merged
merged 19 commits into from
Apr 19, 2021
Merged

V4 is getting ready #866

merged 19 commits into from
Apr 19, 2021

Conversation

nagi1
Copy link
Contributor

@nagi1 nagi1 commented Apr 3, 2021

nagi1 and others added 7 commits March 27, 2021 15:49
In addition to:
- ⚡ Add missing return types to some functions.
- 🎉 Update tests to use the new config object.
 🔧 Fix typo in method name on ActivityLog options.
 🔀 Merge DetectsChanges trait into LogsActivity.
 ♻️ Clean some unnecessary helper functions.
 📝 Document some behaviours.
 ✅ Add 2 new tests to demonstrate powerfulness of the pipeline.
✅ Add LogBatch Facade.
🚑 Fix code styles and dead methods.
@nagi1 nagi1 marked this pull request as draft April 3, 2021 21:39
@nagi1 nagi1 changed the title V4 V4 is getting ready Apr 3, 2021
nagi1 and others added 5 commits April 3, 2021 23:53
- ✅ Add unit test for UserResolver
- ✅ Add couple feature tests
- 💚 Add some types and docblocks
- 🚀 Ability to resolve user in multiple ways including via callbacks
Copy link
Collaborator

@Gummibeer Gummibeer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One general thing to discuss - should we remove the prefixes from the class names ActivityLog as we already have it in namespace!?
For me this feels like duplication and makes the class names long and not in a good way.
The batch for example - we will never add a second batch class. So Spatie\Activitylog\Batch or similar would be enough I think!?

And even if I have a lot of comments/change-requests in here: amazing work you did here! 🎉🚀 And in such a short time.

.github/workflows/run-tests.yml Show resolved Hide resolved
composer.json Outdated Show resolved Hide resolved
composer.json Show resolved Hide resolved
src/ActivitylogServiceProvider.php Outdated Show resolved Hide resolved
src/ActivityLogger.php Outdated Show resolved Hide resolved
src/ActivityLoggerBatch.php Outdated Show resolved Hide resolved
src/ActivitylogOptions.php Outdated Show resolved Hide resolved
src/ActivitylogOptions.php Outdated Show resolved Hide resolved
src/ActivitylogOptions.php Outdated Show resolved Hide resolved
src/EventLogBag.php Outdated Show resolved Hide resolved
@nagi1
Copy link
Contributor Author

nagi1 commented Apr 13, 2021

@Gummibeer, What is left now is rewrite the docs, I have some ideas, but let's finish the review process first.

Copy link
Collaborator

@Gummibeer Gummibeer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some small type adjustments - mostly adding string as a valid causer type to support UUIDs.

src/ActivityLogger.php Outdated Show resolved Hide resolved
src/ActivityLogger.php Outdated Show resolved Hide resolved
src/ActivityLogger.php Outdated Show resolved Hide resolved
src/CauserResolver.php Outdated Show resolved Hide resolved
src/CauserResolver.php Outdated Show resolved Hide resolved
src/CauserResolver.php Outdated Show resolved Hide resolved
src/Facades/CauserResolver.php Outdated Show resolved Hide resolved
src/Facades/CauserResolver.php Outdated Show resolved Hide resolved
src/CauserResolver.php Outdated Show resolved Hide resolved
tests/LogsActivityTest.php Outdated Show resolved Hide resolved
- ✅ Add ability to resolve causer using UUID.
- ✅ Fix types.
@Gummibeer Gummibeer marked this pull request as ready for review April 19, 2021 14:47
Copy link
Collaborator

@Gummibeer Gummibeer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔥🎉🚀🎊💝
Awesome work Ahmed! 🤩

@Gummibeer
Copy link
Collaborator

The mentioned missing documentation: please do it in a separated PR - I will merge that one now to the spatie:v4 branch. 🔥

@nagi1
Copy link
Contributor Author

nagi1 commented Apr 19, 2021

First draft of the new docs will be ready tonight 🔥🔥

nagi1 referenced this pull request in nagi1/laravel-activitylog Apr 24, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment