Handle later calls to the add_to_group_path method #28
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
While the `Asset::add_to_group_path()` method _should_ be used while the Asset is being registered, nothing currently prevents (e.g. an exception) the method from being called on the `Asset` object after registration. Since the path resolution in the `get_root_path` method is done lazily, when the method is called, the fact the Asset has been added to a specific group path should matter. This updates the code to support correctly, in my opinion, calls to the `Asset::add_to_group_path` method after the object registration and take the group path into account when resolving the root path. See the test in this PR to have a better understanding.
lucatume
force-pushed
the
feat/add-to-group-path
branch
from
959ec5b to
c97cd03
Compare
dpanta94
approved these changes
Dec 16, 2024
Member
dpanta94
left a comment
dpanta94
left a comment
There was a problem hiding this comment.
It looks good - I would only ask to also update docs to reflect this change.
The places i have in mind is:
- Docblock of
protected string $group_path_nameproperty - readme.md file related to group path feature.
Replace uses of `file_exists` with `is_file`. While there is functionally no difference between the two in this context, the `is_file` function will use a cached check for a small performance boost.
dpanta94
requested changes
Dec 16, 2024
dpanta94
approved these changes
Dec 16, 2024
borkweb
approved these changes
Dec 16, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
While the
Asset::add_to_group_path()method should be used while theAsset is being registered, nothing currently prevents (e.g. an
exception) the method from being called on the
Assetobject afterregistration.
Since the path resolution in the
get_root_pathmethod is donelazily, when the method is called, the fact the Asset has been added to
a specific group path should matter.
This updates the code to support correctly, in my opinion, calls to the
Asset::add_to_group_pathmethod after the object registration and takethe group path into account when resolving the root path.
See the test in this PR to have a better understanding.