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

making original file info available in callback for adhoc requirements #173

Merged
merged 3 commits into from
Aug 11, 2021

Conversation

swapnilsarwe
Copy link
Contributor

@driesvints & @mallardduck ,

There are certain icon-sets which store the icons in the respective folder with the same file name.
Eg:

https://github.com/google/material-design-icons/tree/master/src/action/3d_rotation/materialicons/24.svg

All the files are named as 24.svg under respective folder with folder having the name of the icon.

From the above example, if I have the original file information available in callback, I can rename the file with an appropriate name using the file object passed in this PR

// from above example, lets say my source directory is dist/src/action/3d_rotation/materialicons/24.svg
Following is the callback method configured in after

$svgNormalization = static function (string $tempFilepath, array $iconSet,  \Symfony\Component\Finder\SplFileInfo $file) {
    ... 
    $fileMeta = explode("/", $file->getRealPath); // eg: dist/src/action/3d_rotation/materialicons/24.svg
    $svgFileName = $fileMeta[count($fileMeta) - 3];
    // now i can use this filename to change the name of the file as I need
    ...
};

Your thoughts?

@mallardduck
Copy link
Contributor

Not a bad idea at all!

I sorta dislike that it has to be a breaking change - since other packages already use 2 parameter closures.
It's also unfortunate that there's no way to have a contract/interface for the callable to make it clear -before runtime- that the generator config will be invalid. It'd be nice if the IDE could be aware and telling us for instance.

🤔

@driesvints
Copy link
Member

@mallardduck There's no breaking change here. Existing closures will continue to work as is.

Thanks @swapnilsarwe, I'll take a more thorough look at this as soon as I find the time.

@mallardduck
Copy link
Contributor

@mallardduck There's no breaking change here. Existing closures will continue to work as is.

🤦 That's right - sorry forgot PHP does that as a feature rather than throwing an error.

@@ -40,7 +40,7 @@ public function generate(): void
$this->filesystem->copy($file->getRealPath(), $pathname);

if (is_callable($set['after'] ?? null)) {
$set['after']($pathname, $set);
$set['after']($pathname, $set, $file);
Copy link
Contributor

Choose a reason for hiding this comment

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

@swapnilsarwe my only thought is around if you want just a string of the path, or the Spl object.
Not sure if it's consequential either way, but could be something worth considering.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure. It works for me - atleast for my current use case. I can simply pass the string $file->getRealPath() instead of object of type SplFileObject.

I was just being little greedy by passing the complete $file object.

I will make the changes and update the PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Have made the changes in the PR

@swapnilsarwe swapnilsarwe force-pushed the callback-with-file-obj branch from 0cdf041 to 81df259 Compare August 6, 2021 08:06
@swapnilsarwe
Copy link
Contributor Author

@mallardduck - have made the changes as you had suggested, i doubt whole object would be needed.

Thanks @swapnilsarwe, I'll take a more thorough look at this as soon as I find the time.

@driesvints take your time, i am using the forked version to generate icons in the specific packages where there is a need. They are just a few.

@driesvints driesvints merged commit a7eb8e5 into blade-ui-kit:1.x Aug 11, 2021
@driesvints
Copy link
Member

I don't mind passing the entire file. Could be useful perhaps for other use cases.

Thanks @swapnilsarwe @mallardduck

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

Successfully merging this pull request may close these issues.

3 participants